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

Michael Eager
2015-03-28T16:56:21Z

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