On 18 Mar 2015 17:58, Michael Eager wrote:
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
>
> +#define COMPRESS_BUF_SIZE (1024*1024)
space around the *
would be nice to have a comment noting where the 1MiB comes from
> +static int
> +decompress_gzip (const char *filename, FILE *tmp)
> +{
> +#ifdef HAVE_ZLIB_H
> + char *buf = xmalloc (COMPRESS_BUF_SIZE);
> + gzFile compressed = gzopen (filename, "r");
> + int count, res;
> +
> + if (buf == NULL || compressed == NULL)
> + {
> + fprintf_filtered (gdb_stderr, _("error copying gzip file\n"));
> + free (buf);
> + return 0;
> + }
> +
> + while ((count = gzread (compressed, buf, COMPRESS_BUF_SIZE)))
> + {
> + res = fwrite (buf, 1, count, tmp);
> + if (res != count)
> + {
> + fprintf_filtered (gdb_stderr, _("error decompressing gzip file\n"));
> + free (buf);
this line needs to use a tab to indent
> +/* If file is compressed, uncompress it into a temporary. */
> +
> +int
> +gdb_uncompress (const char *filename, char **uncompressed_filename)
> +{
> + FILE *handle;
> + struct compressed_file_cache_search search, *found;
> + struct stat st;
> + hashval_t hash;
> + void **slot;
> + static unsigned char buffer[1024];
> + size_t count;
> + enum {NONE, GZIP, BZIP2} file_compression = NONE;
shouldn't this enum be declared outside this func ?
> + if (found)
> + {
> + /* We previously uncompressed the file. */
> + if (found->mtime == st.st_mtime)
> + {
this brace needs to indent w/a tab
> + /* Return file if compressed file not changed. */
> + *uncompressed_filename = found->uncompressed_filename;
> + return 1;
> + }
> + else
> + {
same here
> + /* Delete old uncompressed file. */
> + unlink (found->uncompressed_filename);
> + xfree (found->filename);
> + xfree (found->uncompressed_filename);
> + }
and here
> + /* Create temporary file name for uncompressed file. */
> + if (!(tmpdir = getenv ("TMPDIR")))
> + tmpdir = "/tmp";
> +
> + if (!asprintf (&template, "%s/%s-XXXXXX", tmpdir, basename (filename)))
> + return 0;
> +
> + decomp_fd = mkstemp (template);
ignoring that assigningments in if statements are frowned upon, looks like you
can just use make_temp_file(NULL) from libiberty
you would also leak the fopen() handle here. probably want to go through this
code again looking for such leaks. and maybe try breaking the func up into more
utility related chunks when possible.
> + if (decomp_fd != -1)
> + {
> + decomp_file = fdopen (decomp_fd, "w+b");
FOPEN_WUB
> + if (file_compression == GZIP)
> + {
you've got more missing tabs in this file. i'm going to stop checking.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20150320/0621332a/attachment.sig>