srzip: Avoid low-level FD-based I/O

Use in-memory buffers instead of temporary files. This avoids
the need for low-level I/O on the FD returned by g_mkstemp().
Refactor the code accordingly. Also plug a number of leaks and
tighten the error checking.

diff --git a/src/libsigrok-internal.h b/src/libsigrok-internal.h
index c8940c89ad109ae1adbcc562f5e066af57a34bfb..275deac4ad07c22ef7a99048149e0f34d0e1efd3 100644 (file)
--- a/src/libsigrok-internal.h
+++ b/src/libsigrok-internal.h
@@ -34,6 +34,9 @@
 #include <libserialport.h>
 #endif
 
+struct zip;
+struct zip_stat;
+
 /**
  * @file
  *
@@ -730,6 +733,11 @@ SR_PRIV int sr_packet_copy(const struct sr_datafeed_packet *packet,
                struct sr_datafeed_packet **copy);
 SR_PRIV void sr_packet_free(struct sr_datafeed_packet *packet);
 
+/*--- session_file.c --------------------------------------------------------*/
+
+SR_PRIV GKeyFile *sr_sessionfile_read_metadata(struct zip *archive,
+                       const struct zip_stat *entry);
+
 /*--- analog.c --------------------------------------------------------------*/
 
 SR_PRIV int sr_analog_init(struct sr_datafeed_analog2 *analog,

diff --git a/src/output/srzip.c b/src/output/srzip.c
index d80a7a188cb428feea45d80f91804083f4365970..c562e60401654fe2bd18bd66e678c1e20e529839 100644 (file)
--- a/src/output/srzip.c
+++ b/src/output/srzip.c
@@ -19,7 +19,6 @@
 
 #include <config.h>
 #include <stdlib.h>
-#include <unistd.h>
 #include <string.h>
 #include <errno.h>
 #include <glib.h>
@@ -42,7 +41,7 @@ static int init(struct sr_output *o, GHashTable *options)
 
        (void)options;
 
-       if (strlen(o->filename) == 0) {
+       if (!o->filename || o->filename[0] == '\0') {
                sr_info("srzip output module requires a file name, cannot save.");
                return SR_ERR_ARG;
        }
@@ -57,79 +56,84 @@ static int init(struct sr_output *o, GHashTable *options)
 static int zip_create(const struct sr_output *o)
 {
        struct out_context *outc;
-       struct sr_channel *ch;
-       FILE *meta;
        struct zip *zipfile;
        struct zip_source *versrc, *metasrc;
+       struct sr_channel *ch;
        GVariant *gvar;
+       GKeyFile *meta;
        GSList *l;
-       int tmpfile, ret;
-       char version[1], metafile[32], *s;
+       const char *devgroup;
+       char *s, *metabuf;
+       gsize metalen;
 
        outc = o->priv;
-       if (outc->samplerate == 0) {
-               if (sr_config_get(o->sdi->driver, o->sdi, NULL, SR_CONF_SAMPLERATE,
-                               &gvar) == SR_OK) {
-                       outc->samplerate = g_variant_get_uint64(gvar);
-                       g_variant_unref(gvar);
-               }
+
+       if (outc->samplerate == 0 && sr_config_get(o->sdi->driver, o->sdi, NULL,
+                                       SR_CONF_SAMPLERATE, &gvar) == SR_OK) {
+               outc->samplerate = g_variant_get_uint64(gvar);
+               g_variant_unref(gvar);
        }
 
        /* Quietly delete it first, libzip wants replace ops otherwise. */
-       unlink(outc->filename);
-       if (!(zipfile = zip_open(outc->filename, ZIP_CREATE, &ret)))
+       g_unlink(outc->filename);
+       zipfile = zip_open(outc->filename, ZIP_CREATE, NULL);
+       if (!zipfile)
                return SR_ERR;
 
        /* "version" */
-       version[0] = '2';
-       if (!(versrc = zip_source_buffer(zipfile, version, 1, 0)))
-               return SR_ERR;
-       if (zip_add(zipfile, "version", versrc) == -1) {
-               sr_info("Error saving version into zipfile: %s.",
+       versrc = zip_source_buffer(zipfile, "2", 1, FALSE);
+       if (zip_file_add(zipfile, "version", versrc, 0) < 0) {
+               sr_err("Error saving version into zipfile: %s",
                        zip_strerror(zipfile));
+               zip_source_free(versrc);
+               zip_discard(zipfile);
                return SR_ERR;
        }
 
        /* init "metadata" */
-       strcpy(metafile, "sigrok-meta-XXXXXX");
-       if ((tmpfile = g_mkstemp(metafile)) == -1)
-               return SR_ERR;
-       close(tmpfile);
-       meta = g_fopen(metafile, "wb");
-       fprintf(meta, "[global]\n");
-       fprintf(meta, "sigrok version = %s\n", SR_PACKAGE_VERSION_STRING);
-       fprintf(meta, "[device 1]\ncapturefile = logic-1\n");
-       fprintf(meta, "total probes = %d\n", g_slist_length(o->sdi->channels));
+       meta = g_key_file_new();
+
+       g_key_file_set_string(meta, "global", "sigrok version",
+                       SR_PACKAGE_VERSION_STRING);
+
+       devgroup = "device 1";
+       g_key_file_set_string(meta, devgroup, "capturefile", "logic-1");
+
+       g_key_file_set_integer(meta, devgroup, "total probes",
+                       g_slist_length(o->sdi->channels));
+
        s = sr_samplerate_string(outc->samplerate);
-       fprintf(meta, "samplerate = %s\n", s);
+       g_key_file_set_string(meta, devgroup, "samplerate", s);
        g_free(s);
 
        for (l = o->sdi->channels; l; l = l->next) {
                ch = l->data;
-               if (ch->type != SR_CHANNEL_LOGIC)
-                       continue;
-               if (!ch->enabled)
-                       continue;
-               fprintf(meta, "probe%d = %s\n", ch->index + 1, ch->name);
+               if (ch->enabled && ch->type == SR_CHANNEL_LOGIC) {
+                       s = g_strdup_printf("probe%d", ch->index + 1);
+                       g_key_file_set_string(meta, devgroup, s, ch->name);
+                       g_free(s);
+               }
        }
-       fclose(meta);
+       metabuf = g_key_file_to_data(meta, &metalen, NULL);
+       g_key_file_free(meta);
 
-       if (!(metasrc = zip_source_file(zipfile, metafile, 0, -1))) {
-               unlink(metafile);
-               return SR_ERR;
-       }
-       if (zip_add(zipfile, "metadata", metasrc) == -1) {
-               unlink(metafile);
+       metasrc = zip_source_buffer(zipfile, metabuf, metalen, FALSE);
+       if (zip_file_add(zipfile, "metadata", metasrc, 0) < 0) {
+               sr_err("Error saving metadata into zipfile: %s",
+                       zip_strerror(zipfile));
+               zip_source_free(metasrc);
+               zip_discard(zipfile);
+               g_free(metabuf);
                return SR_ERR;
        }
 
-       if ((ret = zip_close(zipfile)) == -1) {
-               sr_info("Error saving zipfile: %s.", zip_strerror(zipfile));
-               unlink(metafile);
+       if (zip_close(zipfile) < 0) {
+               sr_err("Error saving zipfile: %s", zip_strerror(zipfile));
+               zip_discard(zipfile);
+               g_free(metabuf);
                return SR_ERR;
        }
-
-       unlink(metafile);
+       g_free(metabuf);
 
        return SR_OK;
 }
@@ -140,72 +144,62 @@ static int zip_append(const struct sr_output *o, unsigned char *buf,
        struct out_context *outc;
        struct zip *archive;
        struct zip_source *logicsrc;
-       zip_int64_t num_files;
-       struct zip_file *zf;
+       int64_t i, num_files;
        struct zip_stat zs;
        struct zip_source *metasrc;
        GKeyFile *kf;
        GError *error;
-       gsize len;
-       int chunk_num, next_chunk_num, tmpfile, ret, i;
+       uint64_t chunk_num;
        const char *entry_name;
-       char *metafile, tmpname[32], chunkname[16];
+       char *metabuf;
+       gsize metalen;
+       char *chunkname;
+       unsigned int next_chunk_num;
 
        outc = o->priv;
-       if (!(archive = zip_open(outc->filename, 0, &ret)))
+       if (!(archive = zip_open(outc->filename, 0, NULL)))
                return SR_ERR;
 
-       if (zip_stat(archive, "metadata", 0, &zs) == -1)
+       if (zip_stat(archive, "metadata", 0, &zs) < 0) {
+               sr_err("Failed to open metadata: %s", zip_strerror(archive));
+               zip_discard(archive);
                return SR_ERR;
-
-       metafile = g_malloc(zs.size);
-       zf = zip_fopen_index(archive, zs.index, 0);
-       zip_fread(zf, metafile, zs.size);
-       zip_fclose(zf);
-
+       }
+       kf = sr_sessionfile_read_metadata(archive, &zs);
+       if (!kf) {
+               zip_discard(archive);
+               return SR_ERR_DATA;
+       }
        /*
         * If the file was only initialized but doesn't yet have any
         * data it in, it won't have a unitsize field in metadata yet.
         */
        error = NULL;
-       kf = g_key_file_new();
-       if (!g_key_file_load_from_data(kf, metafile, zs.size, 0, &error)) {
-               sr_err("Failed to parse metadata: %s.", error->message);
-               return SR_ERR;
-       }
-       g_free(metafile);
-       tmpname[0] = '\0';
+       metabuf = NULL;
        if (!g_key_file_has_key(kf, "device 1", "unitsize", &error)) {
                if (error && error->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND) {
-                       sr_err("Failed to check unitsize key: %s", error ? error->message : "?");
+                       sr_err("Failed to check unitsize key: %s", error->message);
+                       g_error_free(error);
+                       g_key_file_free(kf);
+                       zip_discard(archive);
                        return SR_ERR;
                }
+               g_clear_error(&error);
+
                /* Add unitsize field. */
                g_key_file_set_integer(kf, "device 1", "unitsize", unitsize);
-               metafile = g_key_file_to_data(kf, &len, &error);
-               strcpy(tmpname, "sigrok-meta-XXXXXX");
-               if ((tmpfile = g_mkstemp(tmpname)) == -1)
-                       return SR_ERR;
-               if (write(tmpfile, metafile, len) < 0) {
-                       sr_dbg("Failed to create new metadata: %s", g_strerror(errno));
-                       g_free(metafile);
-                       unlink(tmpname);
-                       return SR_ERR;
-               }
-               close(tmpfile);
-               if (!(metasrc = zip_source_file(archive, tmpname, 0, -1))) {
-                       sr_err("Failed to create zip source for metadata.");
-                       g_free(metafile);
-                       unlink(tmpname);
+               metabuf = g_key_file_to_data(kf, &metalen, NULL);
+               metasrc = zip_source_buffer(archive, metabuf, metalen, FALSE);
+
+               if (zip_replace(archive, zs.index, metasrc) < 0) {
+                       sr_err("Failed to replace metadata: %s",
+                               zip_strerror(archive));
+                       g_key_file_free(kf);
+                       zip_source_free(metasrc);
+                       zip_discard(archive);
+                       g_free(metabuf);
                        return SR_ERR;
                }
-               if (zip_replace(archive, zs.index, metasrc) == -1) {
-                       sr_err("Failed to replace metadata file.");
-                       g_free(metafile);
-                       unlink(tmpname);
-                       return SR_ERR;
-               }
-               g_free(metafile);
        }
        g_key_file_free(kf);
 
@@ -213,39 +207,50 @@ static int zip_append(const struct sr_output *o, unsigned char *buf,
        num_files = zip_get_num_entries(archive, 0);
        for (i = 0; i < num_files; i++) {
                entry_name = zip_get_name(archive, i, 0);
-               if (strncmp(entry_name, "logic-1", 7))
+               if (!entry_name || strncmp(entry_name, "logic-1", 7) != 0)
                        continue;
-               if (strlen(entry_name) == 7) {
+               if (entry_name[7] == '\0') {
                        /* This file has no extra chunks, just a single "logic-1".
                         * Rename it to "logic-1-1" * and continue with chunk 2. */
-                       if (zip_rename(archive, i, "logic-1-1") == -1) {
-                               sr_err("Failed to rename 'logic-1' to 'logic-1-1'.");
-                               unlink(tmpname);
+                       if (zip_rename(archive, i, "logic-1-1") < 0) {
+                               sr_err("Failed to rename 'logic-1' to 'logic-1-1': %s",
+                                       zip_strerror(archive));
+                               zip_discard(archive);
+                               g_free(metabuf);
                                return SR_ERR;
                        }
                        next_chunk_num = 2;
                        break;
-               } else if (strlen(entry_name) > 8 && entry_name[7] == '-') {
-                       chunk_num = strtoull(entry_name + 8, NULL, 10);
-                       if (chunk_num >= next_chunk_num)
+               } else if (entry_name[7] == '-') {
+                       chunk_num = g_ascii_strtoull(entry_name + 8, NULL, 10);
+                       if (chunk_num < G_MAXINT && chunk_num >= next_chunk_num)
                                next_chunk_num = chunk_num + 1;
                }
        }
-       snprintf(chunkname, 15, "logic-1-%d", next_chunk_num);
-       if (!(logicsrc = zip_source_buffer(archive, buf, length, FALSE))) {
-               unlink(tmpname);
-               return SR_ERR;
+
+       if (length % unitsize != 0) {
+               sr_warn("Chunk size %d not a multiple of the"
+                       " unit size %d.", length, unitsize);
        }
-       if (zip_add(archive, chunkname, logicsrc) == -1) {
-               unlink(tmpname);
+       logicsrc = zip_source_buffer(archive, buf, length, FALSE);
+       chunkname = g_strdup_printf("logic-1-%u", next_chunk_num);
+       i = zip_add(archive, chunkname, logicsrc);
+       g_free(chunkname);
+       if (i < 0) {
+               sr_err("Failed to add chunk 'logic-1-%u': %s",
+                       next_chunk_num, zip_strerror(archive));
+               zip_source_free(logicsrc);
+               zip_discard(archive);
+               g_free(metabuf);
                return SR_ERR;
        }
-       if ((ret = zip_close(archive)) == -1) {
-               sr_info("error saving session file: %s", zip_strerror(archive));
-               unlink(tmpname);
+       if (zip_close(archive) < 0) {
+               sr_err("Error saving session file: %s", zip_strerror(archive));
+               zip_discard(archive);
+               g_free(metabuf);
                return SR_ERR;
        }
-       unlink(tmpname);
+       g_free(metabuf);
 
        return SR_OK;
 }