zip-reader: Switch to a more usual error reporting mechanism.
authorBen Pfaff <blp@cs.stanford.edu>
Sat, 23 Jan 2021 22:34:03 +0000 (14:34 -0800)
committerBen Pfaff <blp@cs.stanford.edu>
Mon, 25 Jan 2021 04:51:49 +0000 (20:51 -0800)
Having the client pass in a string buffer was clever but it gave the
zip_reader some shared state with its client that will be difficult to
deal with in future use cases.

src/data/ods-reader.c
src/libpspp/zip-reader.c
src/libpspp/zip-reader.h
src/output/spv/spv.c
tests/libpspp/zip-test.c

index bb72e2437c85800319ca3b7cb8b302c8592fb35d..d15c24a238a2e69772acaea5909c94e94893d28a 100644 (file)
@@ -121,7 +121,6 @@ struct ods_reader
 
   struct string ods_errs;
 
-  struct string zip_errs;
   struct hmap cache;
 };
 
@@ -206,10 +205,12 @@ state_data_init (const struct ods_reader *r, struct state_data *sd)
 {
   memset (sd, 0, sizeof (*sd));
 
-  sd->zm = zip_member_open (r->zreader, "content.xml");
-
-  if (sd->zm == NULL)
-    return false;
+  char *error = zip_member_open (r->zreader, "content.xml", &sd->zm);
+  if (error)
+    {
+      free (error);
+      return false;
+    }
 
   sd->xtr =
     xmlReaderForIO (xml_reader_for_zip_member, NULL, sd->zm, NULL, NULL,
@@ -698,10 +699,12 @@ get_sheet_count (struct zip_reader *zreader)
 {
   xmlTextReaderPtr mxtr;
   struct zip_member *meta = NULL;
-  meta = zip_member_open (zreader, "meta.xml");
-
-  if (meta == NULL)
-    return -1;
+  char *error = zip_member_open (zreader, "meta.xml", &meta);
+  if (error)
+    {
+      free (error);
+      return -1;
+    }
 
   mxtr = xmlReaderForIO (xml_reader_for_zip_member, NULL, meta, NULL, NULL, 0);
 
@@ -1119,9 +1122,13 @@ init_reader (struct ods_reader *r, bool report_errors,
 
   if (state)
     {
-      struct zip_member *content = zip_member_open (r->zreader, "content.xml");
+      struct zip_member *content;
+      char *error = zip_member_open (r->zreader, "content.xml", &content);
       if (content == NULL)
-       return NULL;
+        {
+          free (error);
+          return NULL;
+        }
 
       xmlTextReaderPtr xtr = xmlReaderForIO (xml_reader_for_zip_member, NULL, content, NULL, NULL,
                                             report_errors
@@ -1155,20 +1162,17 @@ struct spreadsheet *
 ods_probe (const char *filename, bool report_errors)
 {
   struct ods_reader *r = xzalloc (sizeof *r);
-  struct zip_reader *zr;
 
-  ds_init_empty (&r->zip_errs);
-
-  zr = zip_reader_create (filename, &r->zip_errs);
-
-  if (zr == NULL)
+  struct zip_reader *zr;
+  char *error = zip_reader_create (filename, &zr);
+  if (error)
     {
       if (report_errors)
        {
          msg (ME, _("Cannot open %s as a OpenDocument file: %s"),
-              filename, ds_cstr (&r->zip_errs));
+              filename, error);
        }
-      ds_destroy (&r->zip_errs);
+      free (error);
       free (r);
       return NULL;
     }
@@ -1188,7 +1192,6 @@ ods_probe (const char *filename, bool report_errors)
   return &r->spreadsheet;
 
  error:
-  ds_destroy (&r->zip_errs);
   zip_reader_destroy (r->zreader);
   free (r);
   return NULL;
index 8868c7db338918a36d53bccb0354480c3997c64a..7b5aabadfbf870c9e54899984f33adb3d0edba91 100644 (file)
@@ -48,14 +48,13 @@ struct zip_member
   const struct decompressor *decompressor;
 
   size_t bytes_unread;       /* Number of bytes left in the member available for reading */
-  struct string *errmsgs;    /* A string to hold error messages.
-                               This string is NOT owned by this object. */
+  char *error;               /* Error message, if any. */
   void *aux;
 };
 
 struct decompressor
 {
-  bool (*init) (struct zip_member *);
+  char *(*init) (struct zip_member *);
   int  (*read) (struct zip_member *, void *, size_t);
   void (*finish) (struct zip_member *);
 };
@@ -85,8 +84,6 @@ struct zip_reader
   char *file_name;                  /* The name of the file from which the data is read */
   uint16_t n_entries;              /* Number of directory entries. */
   struct zip_entry *entries;       /* Directory entries. */
-  struct string *errs;             /* A string to hold error messages.  This
-                                      string is NOT owned by this object. */
 };
 
 struct zip_entry
@@ -97,6 +94,14 @@ struct zip_entry
   char *name;                 /* Name of member file. */
 };
 
+char * WARN_UNUSED_RESULT
+zip_member_steal_error (struct zip_member *zm)
+{
+  char *retval = zm->error;
+  zm->error = NULL;
+  return retval;
+}
+
 void
 zip_member_finish (struct zip_member *zm)
 {
@@ -104,9 +109,9 @@ zip_member_finish (struct zip_member *zm)
     {
       free (zm->file_name);
       free (zm->member_name);
-      ds_clear (zm->errmsgs);
       zm->decompressor->finish (zm);
       fclose (zm->fp);
+      free (zm->error);
       free (zm);
     }
 }
@@ -138,61 +143,66 @@ skip_bytes (FILE *f, size_t n)
   fseeko (f, n, SEEK_CUR);
 }
 
-static bool get_bytes (FILE *f, void *x, size_t n) WARN_UNUSED_RESULT;
-
-
-/* Read N bytes from F, storing the result in X */
-static bool
+static void
 get_bytes (FILE *f, void *x, size_t n)
 {
-  return (n == fread (x, 1, n, f));
+  if (!fread (x, n, 1, f))
+    memset (x, 0, n);
 }
 
-/* Read a 32 bit value from F */
-static bool WARN_UNUSED_RESULT
-get_u32 (FILE *f, uint32_t *v)
+static uint32_t
+get_u32 (FILE *f)
 {
   uint32_t x;
-  if (!get_bytes (f, &x, sizeof x))
-    return false;
-  *v = le_to_native32 (x);
-  return true;
+  get_bytes (f, &x, sizeof x);
+  return le_to_native32 (x);
 }
 
-/* Read a 16 bit value from F */
-static bool WARN_UNUSED_RESULT
-get_u16 (FILE *f, uint16_t *v)
+static uint16_t
+get_u16 (FILE *f)
 {
   uint16_t x;
-  if (!get_bytes (f, &x, sizeof x))
-    return false;
-  *v = le_to_native16 (x);
-  return true;
+  get_bytes (f, &x, sizeof x);
+  return le_to_native16 (x);
 }
 
+static char * WARN_UNUSED_RESULT
+get_stream_error (FILE *f, const char *file_name)
+{
+  if (feof (f))
+    return xasprintf (_("%s: unexpected end of file"), file_name);
+  else if (ferror (f))
+    {
+      /* The particular error might not be in errno anymore.  Try to find out
+         what the error was. */
+      errno = 0;
+      char x;
+      return (!fread (&x, 1, sizeof x, f) && errno
+              ? xasprintf (_("%s: I/O error reading Zip archive (%s)"),
+                           file_name, strerror (errno))
+              : xasprintf (_("%s: I/O error reading Zip archive"), file_name));
+    }
+  else
+    return NULL;
+}
 
 /* Read 32 bit integer and compare it with EXPECTED.
    place an error string in ERR if necessary. */
-static bool
-check_magic (FILE *f, const char *file_name,
-             uint32_t expected, struct string *err)
+static char * WARN_UNUSED_RESULT
+check_magic (FILE *f, const char *file_name, uint32_t expected)
 {
-  uint32_t magic;
-
-  if (! get_u32 (f, &magic)) return false;
-
-  if ((expected != magic))
-    {
-      ds_put_format (err,
-                    _("%s: corrupt archive at 0x%llx: "
-                       "expected %#"PRIx32" but got %#"PRIx32),
-                     file_name,
-                    (long long int) ftello (f) - sizeof (uint32_t),
-                     expected, magic);
-
-      return false;
-    }
-  return true;
+  uint32_t magic = get_u32 (f);
+  char *error = get_stream_error (f, file_name);
+  if (error)
+    return error;
+  else if (expected != magic)
+    return xasprintf (_("%s: corrupt archive at 0x%llx: "
+                        "expected %#"PRIx32" but got %#"PRIx32),
+                      file_name,
+                      (long long int) ftello (f) - sizeof (uint32_t),
+                      expected, magic);
+  else
+    return NULL;
 }
 
 
@@ -201,14 +211,12 @@ check_magic (FILE *f, const char *file_name,
 int
 zip_member_read (struct zip_member *zm, void *buf, size_t bytes)
 {
-  int bytes_read = 0;
-
-  ds_clear (zm->errmsgs);
-
   if (bytes > zm->bytes_unread)
     bytes = zm->bytes_unread;
+  if (!bytes)
+    return 0;
 
-  bytes_read  = zm->decompressor->read (zm, buf, bytes);
+  int bytes_read = zm->decompressor->read (zm, buf, bytes);
   if (bytes_read < 0)
     return bytes_read;
 
@@ -224,12 +232,13 @@ char * WARN_UNUSED_RESULT
 zip_member_read_all (struct zip_reader *zr, const char *member_name,
                      void **datap, size_t *np)
 {
-  struct zip_member *zm = zip_member_open (zr, member_name);
-  if (!zm)
+  struct zip_member *zm;
+  char *error = zip_member_open (zr, member_name, &zm);
+  if (error)
     {
       *datap = NULL;
       *np = 0;
-      return ds_steal_cstr (zr->errs);
+      return error;
     }
 
   *datap = xmalloc (zm->ucomp_size);
@@ -240,11 +249,12 @@ zip_member_read_all (struct zip_reader *zr, const char *member_name,
     if (zip_member_read (zm, data + (zm->ucomp_size - zm->bytes_unread),
                          zm->bytes_unread) == -1)
       {
+        char *error = zip_member_steal_error (zm);
         zip_member_finish (zm);
         free (*datap);
         *datap = NULL;
         *np = 0;
-        return ds_steal_cstr (zr->errs);
+        return error;
       }
 
   zip_member_finish (zm);
@@ -254,142 +264,127 @@ zip_member_read_all (struct zip_reader *zr, const char *member_name,
 /* Read a central directory header from FILE and initializes ZE with it.
    Returns true if successful, false otherwise.  On error, appends error
    messages to ERRS. */
-static bool
+static char * WARN_UNUSED_RESULT
 zip_header_read_next (FILE *file, const char *file_name,
-                      struct zip_entry *ze, struct string *errs)
+                      struct zip_entry *ze)
 {
-  uint16_t v, nlen, extralen;
-  uint16_t gp, time, date;
-  uint32_t expected_crc;
-
-  uint16_t clen, diskstart, iattr;
-  uint32_t eattr;
-  uint16_t comp_type;
-
-  if (! check_magic (file, file_name, MAGIC_SOCD, errs))
-    return false;
-
-  if (! get_u16 (file, &v)) return false;
-  if (! get_u16 (file, &v)) return false;
-  if (! get_u16 (file, &gp)) return false;
-  if (! get_u16 (file, &comp_type)) return false;
-  if (! get_u16 (file, &time)) return false;
-  if (! get_u16 (file, &date)) return false;
-  if (! get_u32 (file, &expected_crc)) return false;
-  if (! get_u32 (file, &ze->comp_size)) return false;
-  if (! get_u32 (file, &ze->ucomp_size)) return false;
-  if (! get_u16 (file, &nlen)) return false;
-  if (! get_u16 (file, &extralen)) return false;
-  if (! get_u16 (file, &clen)) return false;
-  if (! get_u16 (file, &diskstart)) return false;
-  if (! get_u16 (file, &iattr)) return false;
-  if (! get_u32 (file, &eattr)) return false;
-  if (! get_u32 (file, &ze->offset)) return false;
+  char *error = check_magic (file, file_name, MAGIC_SOCD);
+  if (error)
+    return error;
+
+  get_u16 (file);       /* v */
+  get_u16 (file);       /* v */
+  get_u16 (file);       /* gp */
+  get_u16 (file);       /* comp_type */
+  get_u16 (file);       /* time */
+  get_u16 (file);       /* date */
+  get_u32 (file);       /* expected_crc */
+  ze->comp_size = get_u32 (file);
+  ze->ucomp_size = get_u32 (file);
+  uint16_t nlen = get_u16 (file);
+  uint16_t extralen = get_u16 (file);
+  get_u16 (file);       /* clen */
+  get_u16 (file);       /* diskstart */
+  get_u16 (file);       /* iattr */
+  get_u32 (file);       /* eattr */
+  ze->offset = get_u32 (file);
+
+  error = get_stream_error (file, file_name);
+  if (error)
+    return error;
 
   ze->name = xzalloc (nlen + 1);
-  if (! get_bytes (file, ze->name, nlen)) return false;
+  get_bytes (file, ze->name, nlen);
+  error = get_stream_error (file, file_name);
+  if (error)
+    return error;
 
   skip_bytes (file, extralen);
 
-  return true;
+  return NULL;
 }
 
 
 /* Create a reader from the zip called FILE_NAME */
-struct zip_reader *
-zip_reader_create (const char *file_name, struct string *errs)
+char * WARN_UNUSED_RESULT
+zip_reader_create (const char *file_name, struct zip_reader **zrp)
 {
-  uint16_t disknum, n_members, total_members;
-  off_t offset = 0;
-  uint32_t central_dir_start, central_dir_length;
-
-  struct zip_reader *zr = xzalloc (sizeof *zr);
-  zr->errs = errs;
-  if (zr->errs)
-    ds_init_empty (zr->errs);
+  *zrp = NULL;
 
   FILE *file = fopen (file_name, "rb");
   if (!file)
-    {
-      ds_put_format (zr->errs, _("%s: open failed (%s)"),
-                     file_name, strerror (errno));
-      free (zr);
-      return NULL;
-    }
+    return xasprintf (_("%s: open failed (%s)"), file_name, strerror (errno));
 
-  if (! check_magic (file, file_name, MAGIC_LHDR, zr->errs))
+  /* Check the Zip file magic. */
+  char *error = check_magic (file, file_name, MAGIC_LHDR);
+  if (error)
     {
       fclose (file);
-      free (zr);
-      return NULL;
+      return error;
     }
 
+  /* Find end of central directory record and read it. */
+  off_t offset = 0;
   if (! find_eocd (file, &offset))
     {
-      ds_put_format (zr->errs, _("%s: cannot find central directory"),
-                     file_name);
       fclose (file);
-      free (zr);
-      return NULL;
+      return xasprintf (_("%s: cannot find central directory"), file_name);
     }
-
   if (0 != fseeko (file, offset, SEEK_SET))
     {
-      ds_put_format (zr->errs, _("%s: seek failed (%s)"),
-                     file_name, strerror (errno));
+      error = xasprintf (_("%s: seek failed (%s)"),
+                         file_name, strerror (errno));
       fclose (file);
-      free (zr);
-      return NULL;
+      return error;
     }
-
-
-  if (! check_magic (file, file_name, MAGIC_EOCD, zr->errs))
+  error = check_magic (file, file_name, MAGIC_EOCD);
+  if (error)
     {
       fclose (file);
-      free (zr);
-      return NULL;
+      return error;
     }
-
-  if (! get_u16 (file, &disknum)
-      || ! get_u16 (file, &disknum)
-
-      || ! get_u16 (file, &n_members)
-      || ! get_u16 (file, &total_members)
-
-      || ! get_u32 (file, &central_dir_length)
-      || ! get_u32 (file, &central_dir_start))
+  get_u16 (file);               /* disknum */
+  get_u16 (file);               /* disknum */
+  uint16_t n_members = get_u16 (file);
+  get_u16 (file);               /* total_members */
+  get_u32 (file);               /* central_dir_length */
+  uint32_t central_dir_start = get_u32 (file);
+  error = get_stream_error (file, file_name);
+  if (error)
     {
       fclose (file);
-      free (zr);
-      return NULL;
+      return error;
     }
 
+  /* Read central directory. */
   if (0 != fseeko (file, central_dir_start, SEEK_SET))
     {
-      ds_put_format (zr->errs, _("%s: seek failed (%s)"),
-                     file_name, strerror (errno));
+      error = xasprintf (_("%s: seek failed (%s)"),
+                         file_name, strerror (errno));
       fclose (file);
-      free (zr);
       return NULL;
     }
 
+  struct zip_reader *zr = xzalloc (sizeof *zr);
   zr->file_name = xstrdup (file_name);
-
   zr->entries = xcalloc (n_members, sizeof *zr->entries);
   for (int i = 0; i < n_members; i++)
     {
-      if (!zip_header_read_next (file, file_name,
-                                 &zr->entries[zr->n_entries], errs))
+      error = zip_header_read_next (file, file_name,
+                                    &zr->entries[zr->n_entries]);
+      if (error)
         {
           fclose (file);
           zip_reader_destroy (zr);
-          return NULL;
+          return error;
         }
       zr->n_entries++;
     }
 
   fclose (file);
-  return zr;
+
+  *zrp = zr;
+  return NULL;
 }
 
 static struct zip_entry *
@@ -418,24 +413,21 @@ zip_reader_contains_member (const struct zip_reader *zr, const char *member)
 }
 
 /* Return the member called MEMBER from the reader ZR  */
-struct zip_member *
-zip_member_open (struct zip_reader *zr, const char *member)
+char * WARN_UNUSED_RESULT
+zip_member_open (struct zip_reader *zr, const char *member,
+                 struct zip_member **zmp)
 {
+  *zmp = NULL;
+
   struct zip_entry *ze = zip_entry_find (zr, member);
   if (ze == NULL)
-    {
-      ds_put_format (zr->errs, _("%s: unknown member \"%s\""),
-                     zr->file_name, member);
-      return NULL;
-    }
+    return xasprintf (_("%s: unknown member \"%s\""),
+                        zr->file_name, member);
 
   FILE *fp = fopen (zr->file_name, "rb");
   if (!fp)
-    {
-      ds_put_format (zr->errs, _("%s: open failed (%s)"),
-                     zr->file_name, strerror (errno));
-      return NULL;
-    }
+    return xasprintf ( _("%s: open failed (%s)"),
+                       zr->file_name, strerror (errno));
 
   struct zip_member *zm = xmalloc (sizeof *zm);
   zm->file_name = xstrdup (zr->file_name);
@@ -446,49 +438,53 @@ zip_member_open (struct zip_reader *zr, const char *member)
   zm->ucomp_size = ze->ucomp_size;
   zm->decompressor = NULL;
   zm->bytes_unread = ze->ucomp_size;
-  zm->errmsgs = zr->errs;
   zm->aux = NULL;
+  zm->error = NULL;
 
+  char *error;
   if (0 != fseeko (zm->fp, zm->offset, SEEK_SET))
     {
-      ds_put_format (zr->errs, _("%s: seek failed (%s)"),
-                     ze->name, strerror (errno));
+      error = xasprintf (_("%s: seek failed (%s)"),
+                         ze->name, strerror (errno));
       goto error;
     }
 
-  if (! check_magic (zm->fp, zr->file_name, MAGIC_LHDR, zr->errs))
+  error = check_magic (zm->fp, zr->file_name, MAGIC_LHDR);
+  if (error)
     goto error;
 
-  uint16_t v, nlen, extra_len;
-  uint16_t gp, comp_type, time, date;
-  uint32_t ucomp_size, comp_size;
-  uint32_t crc;
-  if (! get_u16 (zm->fp, &v)) goto error;
-  if (! get_u16 (zm->fp, &gp)) goto error;
-  if (! get_u16 (zm->fp, &comp_type)) goto error;
+  get_u16 (zm->fp);             /* v */
+  get_u16 (zm->fp);             /* gp */
+  uint16_t comp_type = get_u16 (zm->fp);
   zm->decompressor = get_decompressor (comp_type);
-  if (! zm->decompressor) goto error;
-  if (! get_u16 (zm->fp, &time)) goto error;
-  if (! get_u16 (zm->fp, &date)) goto error;
-  if (! get_u32 (zm->fp, &crc)) goto error;
-  if (! get_u32 (zm->fp, &comp_size)) goto error;
-
-  if (! get_u32 (zm->fp, &ucomp_size)) goto error;
-  if (! get_u16 (zm->fp, &nlen)) goto error;
-  if (! get_u16 (zm->fp, &extra_len)) goto error;
-
-  char *name = xzalloc (nlen + 1);
-  if (! get_bytes (zm->fp, name, nlen))
+  if (!zm->decompressor)
     {
-      free (name);
+      error = xasprintf (_("%s: member \"%s\" has unknown compression "
+                           "type %"PRIu16),
+                         zr->file_name, zm->member_name, comp_type);
       goto error;
     }
+  get_u16 (zm->fp);             /* time */
+  get_u16 (zm->fp);             /* date */
+  get_u32 (zm->fp);             /* crc */
+  get_u32 (zm->fp);             /* comp_size */
+  get_u32 (zm->fp);             /* ucomp_size */
+  uint16_t nlen = get_u16 (zm->fp);
+  uint16_t extra_len = get_u16 (zm->fp);
+  error = get_stream_error (zm->fp, zr->file_name);
+  if (error)
+    goto error;
+
+  char *name = xzalloc (nlen + 1);
+  get_bytes (zm->fp, name, nlen);
+  error = get_stream_error (zm->fp, zr->file_name);
+  if (error)
+    goto error;
   if (strcmp (name, ze->name) != 0)
     {
-      ds_put_format (zm->errmsgs,
-                    _("%s: name mismatch between central directory (%s) "
-                       "and local file header (%s)"),
-                     zm->file_name, ze->name, name);
+      error = xasprintf (_("%s: name mismatch between central directory (%s) "
+                           "and local file header (%s)"),
+                         zm->file_name, ze->name, name);
       free (name);
       goto error;
     }
@@ -496,17 +492,19 @@ zip_member_open (struct zip_reader *zr, const char *member)
 
   skip_bytes (zm->fp, extra_len);
 
-  if (!zm->decompressor->init (zm))
+  error = zm->decompressor->init (zm);
+  if (error)
     goto error;
 
-  return zm;
+  *zmp = zm;
+  return NULL;
 
 error:
   fclose (zm->fp);
   free (zm->file_name);
   free (zm->member_name);
   free (zm);
-  return NULL;
+  return error;
 }
 
 \f
@@ -602,13 +600,16 @@ probe_magic (FILE *fp, uint32_t magic, off_t start, off_t stop, off_t *off)
 static int
 stored_read (struct zip_member *zm, void *buf, size_t n)
 {
-  return fread (buf, 1, n, zm->fp);
+  size_t bytes_read = fread (buf, 1, n, zm->fp);
+  if (!bytes_read && !zm->error)
+    zm->error = get_stream_error (zm->fp, zm->file_name);
+  return bytes_read;
 }
 
-static bool
+static char *
 stored_init (struct zip_member *zm UNUSED)
 {
-  return true;
+  return NULL;
 }
 
 static void
@@ -649,7 +650,7 @@ inflate_finish (struct zip_member *zm)
   free (inf);
 }
 
-static bool
+static char *
 inflate_init (struct zip_member *zm)
 {
   int r;
@@ -677,16 +678,12 @@ inflate_init (struct zip_member *zm)
   r = inflateInit (&inf->zss);
 
   if (Z_OK != r)
-    {
-      ds_put_format (zm->errmsgs,
-                     _("%s: cannot initialize inflator (%s)"),
-                     zm->file_name, zError (r));
-      return false;
-    }
+    return xasprintf (_("%s: cannot initialize inflator (%s)"),
+                      zm->file_name, zError (r));
 
   zm->aux = inf;
 
-  return true;
+  return NULL;
 }
 
 static int
@@ -719,6 +716,11 @@ inflate_read (struct zip_member *zm, void *buf, size_t n)
        bytes_to_read = UCOMPSIZE;
 
       bytes_read = fread (inf->ucomp + pad, 1, bytes_to_read - pad, zm->fp);
+      if (!bytes_read && !zm->error)
+        {
+          zm->error = get_stream_error (zm->fp, zm->file_name);
+          return -1;
+        }
 
       inf->ucomp_bytes_read += bytes_read;
 
@@ -734,8 +736,9 @@ inflate_read (struct zip_member *zm, void *buf, size_t n)
       return n - inf->zss.avail_out;
     }
 
-  ds_put_format (zm->errmsgs, _("%s: error inflating \"%s\" (%s)"),
-                 zm->file_name, zm->member_name, zError (r));
+  if (!zm->error)
+    zm->error = xasprintf (_("%s: error inflating \"%s\" (%s)"),
+                           zm->file_name, zm->member_name, zError (r));
 
   return -1;
 }
index 6a65f58c1c95f13d8b7345662f33f796663483dd..b02981d365f81a9235f7827a6276ba089e9f7a86 100644 (file)
 #ifndef ZIP_READER_H
 #define ZIP_READER_H 1
 
+#include "libpspp/compiler.h"
+
 struct zip_member;
 struct zip_reader;
 struct string;
 
-/* Create zip reader to read the file called FILENAME.
-   If ERRS is non-null if will be used to contain any error messages
-   which the reader wishes to report.
- */
-struct zip_reader *zip_reader_create (const char *filename, struct string *errs);
+/* Create zip reader to read the file called FILENAME.  If successful, stores
+   the new zip_reader in *ZRP and returns NULL; on error, returns an error
+   message that the caller must free and stores NULL in *ZRP. */
+char *zip_reader_create (const char *filename, struct zip_reader **zrp)
+  WARN_UNUSED_RESULT;
 
 /* Destroy the zip reader */
 void zip_reader_destroy (struct zip_reader *zr);
@@ -40,11 +42,15 @@ const char *zip_reader_get_member_name(const struct zip_reader *zr,
 bool zip_reader_contains_member (const struct zip_reader *zr,
                                  const char *member);
 
-/* Return the zip member in the reader ZR, called MEMBER */
-struct zip_member *zip_member_open (struct zip_reader *zr, const char *member);
+/* Opens the zip member named MEMBER in ZR.  If successful, stores the new
+   zip_member in *ZMP and returns NULL; on error, returns an error message that
+   the caller must free and stores NULL in *ZMP. */
+char *zip_member_open (struct zip_reader *zr, const char *member,
+                       struct zip_member **zmp) WARN_UNUSED_RESULT;
 
-/* Read up to N bytes from ZM, storing them in BUF.
-   Returns the number of bytes read, or -1 on error */
+/* Read up to N bytes from ZM, storing them in BUF.  Returns the number of
+   bytes read, or -1 on error.  On error, zip_member_steal_error() may be used
+   to obtain an error message. */
 int zip_member_read (struct zip_member *zm, void *buf, size_t n);
 
 /* Read all of ZM into memory, storing the data in *DATAP and its size in *NP.
@@ -53,6 +59,10 @@ int zip_member_read (struct zip_member *zm, void *buf, size_t n);
 char *zip_member_read_all (struct zip_reader *, const char *member_name,
                            void **datap, size_t *np) WARN_UNUSED_RESULT;
 
+/* Returns the error message in ZM (and clears it out of ZM).  The caller must
+   eventually free the returned string. */
+char *zip_member_steal_error (struct zip_member *zm) WARN_UNUSED_RESULT;
+
 void zip_member_finish (struct zip_member *zm);
 
 
index fdafb5aba7c96c3ef3cb0ff9a815b2360257a96a..e23bb1b1960eba26a964fc1fc69647802dfde671 100644 (file)
@@ -55,7 +55,6 @@
 
 struct spv_reader
   {
-    struct string zip_errs;
     struct zip_reader *zip;
     struct spv_item *root;
     struct page_setup *page_setup;
@@ -229,12 +228,12 @@ spv_item_get_image (const struct spv_item *item_)
 
   if (!item->image)
     {
-      struct zip_member *zm = zip_member_open (item->spv->zip,
-                                               item->png_member);
+      struct zip_member *zm;
+      char *error = zip_member_open (item->spv->zip, item->png_member, &zm);
       item->image = cairo_image_surface_create_from_png_stream (
         read_from_zip_member, zm);
-      if (zm)
-        zip_member_finish (zm);
+      zip_member_finish (zm);
+      free (error);
     }
 
   return item->image;
@@ -729,9 +728,10 @@ spv_read_xml_member (struct spv_reader *spv, const char *member_name,
 {
   *docp = NULL;
 
-  struct zip_member *zm = zip_member_open (spv->zip, member_name);
-  if (!zm)
-    return ds_steal_cstr (&spv->zip_errs);
+  struct zip_member *zm;
+  char *error = zip_member_open (spv->zip, member_name, &zm);
+  if (error)
+    return error;
 
   xmlParserCtxt *parser;
   xmlKeepBlanksDefault (keep_blanks);
@@ -754,7 +754,7 @@ spv_read_xml_member (struct spv_reader *spv, const char *member_name,
 
   if (retval < 0)
     {
-      char *error = ds_steal_cstr (&spv->zip_errs);
+      char *error = zip_member_steal_error (zm);
       zip_member_finish (zm);
       xmlFreeDoc (doc);
       return error;
@@ -1153,16 +1153,14 @@ spv_detect__ (struct zip_reader *zip, char **errorp)
 char * WARN_UNUSED_RESULT
 spv_detect (const char *filename)
 {
-  struct string zip_error;
-  struct zip_reader *zip = zip_reader_create (filename, &zip_error);
-  if (!zip)
-    return ds_steal_cstr (&zip_error);
+  struct zip_reader *zip;
+  char *error = zip_reader_create (filename, &zip);
+  if (error)
+    return error;
 
-  char *error;
   if (spv_detect__ (zip, &error) <= 0 && !error)
     error = xasprintf("%s: not an SPV file", filename);
   zip_reader_destroy (zip);
-  ds_destroy (&zip_error);
   return error;
 }
 
@@ -1172,16 +1170,13 @@ spv_open (const char *filename, struct spv_reader **spvp)
   *spvp = NULL;
 
   struct spv_reader *spv = xzalloc (sizeof *spv);
-  ds_init_empty (&spv->zip_errs);
-  spv->zip = zip_reader_create (filename, &spv->zip_errs);
-  if (!spv->zip)
+  char *error = zip_reader_create (filename, &spv->zip);
+  if (error)
     {
-      char *error = ds_steal_cstr (&spv->zip_errs);
       spv_close (spv);
       return error;
     }
 
-  char *error;
   int detect = spv_detect__ (spv->zip, &error);
   if (detect <= 0)
     {
@@ -1220,7 +1215,6 @@ spv_close (struct spv_reader *spv)
 {
   if (spv)
     {
-      ds_destroy (&spv->zip_errs);
       zip_reader_destroy (spv->zip);
       spv_item_destroy (spv->root);
       page_setup_destroy (spv->page_setup);
index 777b37a795259c2f4ca93062e8378d197cfd9140..6590eb8e7817694933f4841e86e66cdee88badf1 100644 (file)
@@ -65,17 +65,16 @@ main (int argc, char **argv)
       const int BUFSIZE=256;
       char buf[BUFSIZE];
       int i;
-      struct string str;
-      struct zip_reader *zr = zip_reader_create (argv[2], &str);
-      if (NULL == zr)
+      struct zip_reader *zr;
+      char *error = zip_reader_create (argv[2], &zr);
+      if (error)
        {
-         fprintf (stderr, "Could not create zip reader: %s\n", ds_cstr (&str));
+         fprintf (stderr, "Could not create zip reader: %s\n", error);
          check_die ();
        }
       for (i = 3; i < argc; ++i)
        {
          int x = 0;
-         struct zip_member *zm ;
          FILE *fp = fopen (argv[i], "wb");
          if (NULL == fp)
            {
@@ -83,11 +82,13 @@ main (int argc, char **argv)
              fprintf (stderr, "Could not create file %s: %s\n", argv[i], strerror(e));
              check_die ();
            }
-         zm = zip_member_open (zr, argv[i]);
-         if (NULL == zm)
+
+         struct zip_member *zm ;
+         char *error = zip_member_open (zr, argv[i], &zm);
+         if (error)
            {
              fprintf (stderr, "Could not open zip member %s from archive: %s\n",
-                      argv[i], ds_cstr (&str));
+                      argv[i], error);
              check_die ();
            }
 
@@ -95,11 +96,14 @@ main (int argc, char **argv)
            {
              fwrite (buf, x, 1, fp);
            }
+          error = zip_member_steal_error (zm);
           zip_member_finish (zm);
          fclose (fp);
+
+          assert ((error != NULL) == (x < 0));
          if (x < 0)
            {
-             fprintf (stderr, "Unzip failed: %s\n", ds_cstr (&str));
+             fprintf (stderr, "Unzip failed: %s\n", error);
              check_die ();
            }
        }