From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49034 invoked by alias); 11 Mar 2015 15:00:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 49003 invoked by uid 89); 11 Mar 2015 15:00:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: homiemail-a43.g.dreamhost.com Received: from sub5.mail.dreamhost.com (HELO homiemail-a43.g.dreamhost.com) (208.113.200.129) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Mar 2015 15:00:42 +0000 Received: from homiemail-a43.g.dreamhost.com (localhost [127.0.0.1]) by homiemail-a43.g.dreamhost.com (Postfix) with ESMTP id 88FAA8C06B; Wed, 11 Mar 2015 08:00:41 -0700 (PDT) Received: from redwood.eagercon.com (c-24-7-16-38.hsd1.ca.comcast.net [24.7.16.38]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: eager@eagerm.com) by homiemail-a43.g.dreamhost.com (Postfix) with ESMTPSA id 387CB8C072; Wed, 11 Mar 2015 08:00:41 -0700 (PDT) Message-ID: <55005898.9040002@eagerm.com> Date: Wed, 11 Mar 2015 15:00:00 -0000 From: Michael Eager User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: "gdb-patches@sourceware.org" , binutils , Mike Frysinger Subject: Re: [PATCH] Support gzip compressed exec and core files in gdb References: <54FF77D6.7010400@eagerm.com> <20150311023755.GX9455@vapier> In-Reply-To: <20150311023755.GX9455@vapier> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00299.txt.bz2 On 03/10/15 19:37, Mike Frysinger wrote: > 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 Thanks for the review. I'll clean up and resubmit. -- Michael Eager eager@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077