This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]