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

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]