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

[Michael Eager <eager@eagerm.com>]

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