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

[Michael Eager <eager@eagerm.com>]

On 03/27/15 08:26, Michael Eager wrote:
> Removed binutils@sourceware.org.
>
> On 03/20/15 15:16, Mike Frysinger wrote:
>> 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
>
> I added a comment, but the buffer size is arbitrary.
>
>>> +  /* 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
>
> Fixed assignment.  make_temp_file() generates an anonymous file; I want to
> keep the user-specified file name as part of the uncompressed file name.
> (I could update libiberty to add an alternate to make_temp_file() which
> takes a basename, I guess.)  I replaced getenv("TMPDIR") with choose_tmpdir()
> 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.
>
> I refactored the gdb_uncompress() function and broke it into 3 functions.
>
>> you've got more missing tabs in this file.  i'm going to stop checking.
>
> I cleaned up the white space.  I hope I got all the tabs this time.
>
> gdb/ChangeLog:
>    * utils.c (struct compressed_file_cache_search, eq_compressed_file,
>    is_gzip, decompress_gzip, do_compressed_cleanup, identify_compression,
>    uncompress_to_temporary, gdb_uncompress): New.
>    * utils.h (gdb_uncompress): Declare.
>    * corelow.c (core_open): Uncompress core file.
>    * exec.c (exec_file_attach): Uncompress exec file.
>    * symfile.c (symfile_bfd_open): Uncompress sym (exec) file.
>    * NEWS: Mention new functionality.
>
> gdb/doc:
>    * gdb.texinfo (Files): Mention gzipped exec and core files.
>
>
> I'm not fond of whitespace patches, but there were many whitespace errors
> in these files unrelated to my patch.  I've attached a second patch to clean
> this up.
>
> gdb/ChangeLog:
> * corelow.c: Replace leading whitespace with tabs, eliminate trailing blanks.
> * exec.c: Ditto.
> * symfile.c: Ditto.
> * utils.c: Ditto
> * utils.h: Ditto

Pedro, Doug -- Can you approve this patch?


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