Re: [PATCH] Support gzip compressed exec and core files in gdb

Michael Eager
2015-03-11T15:00:44Z

On 03/10/15 19:37, Mike Frysinger wrote:
> On 10 Mar 2015 16:01, Michael Eager wrote:
>> --- a/gdb/common/filestuff.c
>> +++ b/gdb/common/filestuff.c
>>
>> +#define COMPRESS_BUF_SIZE (1024*1024)
>> +static int
>> +decompress_gzip (const char *filename, FILE *tmp)
>> +{
>> +#ifdef HAVE_ZLIB_H
>> + char *buf = malloc (COMPRESS_BUF_SIZE);
>
> xmalloc ?
>
>> + if (buf == NULL || compressed == NULL)
>> + {
>> + printf (_("error copying gzip file\n"));
>
> fprintf_filtered (gdb_stderr, ...) ?
>
>> + printf (_("error decompressing gzip file\n"));
>
> here too ?
>
>> + free (buf);
>
> indentation is broken. this comes up a lot, so you should scan the whole thing.
>
>> + fflush (tmp);
>
> that needed ?
>
>> +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;
>> + int decomp_fd;
>> + FILE *decomp_file;
>> + int ret = 0;
>> + char *tmpdir, *p;
>> + char *template = xmalloc(128 + 12 + 7 + 1);
>
> probably should be a comment as to the constants you've written here
>
>> + if (compressed_file_cache == NULL)
>
> style says there should be a blank line above this if statement
>
>> + compressed_file_cache = htab_create_alloc (1, htab_hash_string,
>> + eq_compressed_file,
>> + NULL, xcalloc, xfree);
>> +
>> + if ((stat (filename, &st) < 0))
>
> excess set of paren
>
>> + /* Return file if compressed file not changed. */
>> + *uncompressed_filename = found->uncompressed_filename;
>> + return 1;
>> + }
>> + else
>> + {
>> + /* Delete old uncompressed file. */
>> + unlink (found->uncompressed_filename);
>> + xfree ((void *)found->filename);
>
> is that cast really needed ?
>
>> + if ((handle = fopen (filename, "rb")) == NULL)
>
> gdb generally doesn't like to pack assignments into if statements
>
> use FOPEN_RB instead of "rb" ?
>
>> + /* Create temporary file name for uncompressed file. */
>> + if (!(tmpdir = getenv ("TMPDIR")))
>> + tmpdir = "/tmp";
>> + strncpy (template, tmpdir, 128);
>> + strcat (template, "/");
>> + for (p = (char *)filename + strlen (filename) - 1;
>> + p >= filename && *p != '/'; p--) /* find final slash. */ ;
>> + strncat (template, ++p, 128);
>> + p = template + strlen (template);
>> + if (strcmp (p - 3, ".gz") == 0)
>> + *(p - 3) = '\0';
>> + strcat (template, "-XXXXXX");
>
> that's pretty messy man. why not use mkstemp() and fdopen() instead ?
>
> in general, there's no need for all this strcat business. asprintf allows yout
> easily concat paths & allocate the right amount of space.
> -mike

Thanks for the review. I'll clean up and resubmit.

--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077