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

[Mike Frysinger <vapier at gentoo dot org>]

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

Attachment: signature.asc
Description: Digital signature