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

Mike Frysinger
2015-03-11T02:37:59Z

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

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]