diff mbox series

[ovs-dev,10/13] log: Support multiple magic.

Message ID 20171208001240.25829-11-blp@ovn.org
State Accepted
Headers show
Series OVSDB log enhancements | expand

Commit Message

Ben Pfaff Dec. 8, 2017, 12:12 a.m. UTC 
Some OVSDB tools will want to open files that might be standalone or
clustered databases, and so it's better if ovsdb_log_open() can accept more
than one valid "magic".  This commit makes that possible.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/log.c        | 147 +++++++++++++++++++++++++++++++++++++----------------
 ovsdb/log.h        |   6 ++-
 tests/ovsdb-log.at |   2 +-
 3 files changed, 108 insertions(+), 47 deletions(-)

Comments

Justin Pettit Dec. 23, 2017, 11:41 p.m. UTC | #1 
> On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> diff --git a/ovsdb/log.c b/ovsdb/log.c
> index d25f766474dc..dd641884a2ee 100644
> --- a/ovsdb/log.c
> +++ b/ovsdb/log.c
> 
> +    /* Read the magic from the first log record. */
> +    char header[128];
> +    const char *actual_magic;
> +    if (!fgets(header, sizeof header, stream)) {
> +        if (ferror(stream)) {
> +            error = ovsdb_io_error(errno, "%s: read error", name);
> +            goto error_fclose;
> +        }
> +
> +        /* We need to be able to report what kind of file this is but we can't
> +         * if it's empty and we accept more than one. */
> +        if (strchr(magic, '|')) {
> +            error = ovsdb_error(NULL, "%s: unexpected end of file", name);

An error message indicating that multiple magic values were provided to a new file seems like it might be more helpful.

> void
> ovsdb_log_close(struct ovsdb_log *file)
> {
>     if (file) {
>         ovsdb_error_destroy(file->error);
>         free(file->name);
> +        free(file->magic);

I think this should be added in the first patch of the series.

> static bool
> +parse_header(char *header, const char **magicp,
> +             unsigned long int *length, uint8_t sha1[SHA1_DIGEST_SIZE])

Do you think it's worth pointing out that 'magic' is a pointer into 'header', so it shouldn't be freed?  Also, it looks to be making modifications to 'header'.

> diff --git a/ovsdb/log.h b/ovsdb/log.h
> index cc8469c01e57..4b74ca11ce6a 100644
> --- a/ovsdb/log.h
> +++ b/ovsdb/log.h
> 
> @@ -31,7 +31,7 @@ enum ovsdb_log_open_mode {
>     OVSDB_LOG_CREATE            /* Create or open file, read/write. */
> };
> 
> -#define OVSDB_MAGIC "OVSDB JSON"
> +#define OVSDB_MAGIC "JSON"

It might be worth mentioning that "OVSDB " will be added to the definition.

--Justin
Justin Pettit Dec. 24, 2017, 12:11 a.m. UTC | #2 
I forgot to add:

Acked-by: Justin Pettit <jpettit@ovn.org>


> On Dec 23, 2017, at 4:41 PM, Justin Pettit <jpettit@ovn.org> wrote:
> 
> 
>> On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp@ovn.org> wrote:
>> 
>> diff --git a/ovsdb/log.c b/ovsdb/log.c
>> index d25f766474dc..dd641884a2ee 100644
>> --- a/ovsdb/log.c
>> +++ b/ovsdb/log.c
>> 
>> +    /* Read the magic from the first log record. */
>> +    char header[128];
>> +    const char *actual_magic;
>> +    if (!fgets(header, sizeof header, stream)) {
>> +        if (ferror(stream)) {
>> +            error = ovsdb_io_error(errno, "%s: read error", name);
>> +            goto error_fclose;
>> +        }
>> +
>> +        /* We need to be able to report what kind of file this is but we can't
>> +         * if it's empty and we accept more than one. */
>> +        if (strchr(magic, '|')) {
>> +            error = ovsdb_error(NULL, "%s: unexpected end of file", name);
> 
> An error message indicating that multiple magic values were provided to a new file seems like it might be more helpful.
> 
>> void
>> ovsdb_log_close(struct ovsdb_log *file)
>> {
>>    if (file) {
>>        ovsdb_error_destroy(file->error);
>>        free(file->name);
>> +        free(file->magic);
> 
> I think this should be added in the first patch of the series.
> 
>> static bool
>> +parse_header(char *header, const char **magicp,
>> +             unsigned long int *length, uint8_t sha1[SHA1_DIGEST_SIZE])
> 
> Do you think it's worth pointing out that 'magic' is a pointer into 'header', so it shouldn't be freed?  Also, it looks to be making modifications to 'header'.
> 
>> diff --git a/ovsdb/log.h b/ovsdb/log.h
>> index cc8469c01e57..4b74ca11ce6a 100644
>> --- a/ovsdb/log.h
>> +++ b/ovsdb/log.h
>> 
>> @@ -31,7 +31,7 @@ enum ovsdb_log_open_mode {
>>    OVSDB_LOG_CREATE            /* Create or open file, read/write. */
>> };
>> 
>> -#define OVSDB_MAGIC "OVSDB JSON"
>> +#define OVSDB_MAGIC "JSON"
> 
> It might be worth mentioning that "OVSDB " will be added to the definition.
> 
> --Justin
> 
>
Ben Pfaff Dec. 24, 2017, 9:58 p.m. UTC | #3 
On Sat, Dec 23, 2017 at 04:41:50PM -0700, Justin Pettit wrote:
> 
> > On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > diff --git a/ovsdb/log.c b/ovsdb/log.c
> > index d25f766474dc..dd641884a2ee 100644
> > --- a/ovsdb/log.c
> > +++ b/ovsdb/log.c
> > 
> > +    /* Read the magic from the first log record. */
> > +    char header[128];
> > +    const char *actual_magic;
> > +    if (!fgets(header, sizeof header, stream)) {
> > +        if (ferror(stream)) {
> > +            error = ovsdb_io_error(errno, "%s: read error", name);
> > +            goto error_fclose;
> > +        }
> > +
> > +        /* We need to be able to report what kind of file this is but we can't
> > +         * if it's empty and we accept more than one. */
> > +        if (strchr(magic, '|')) {
> > +            error = ovsdb_error(NULL, "%s: unexpected end of file", name);
> 
> An error message indicating that multiple magic values were provided to a new file seems like it might be more helpful.

Thanks.  I improved the error message here and a little later in the
same function.

> > void
> > ovsdb_log_close(struct ovsdb_log *file)
> > {
> >     if (file) {
> >         ovsdb_error_destroy(file->error);
> >         free(file->name);
> > +        free(file->magic);
> 
> I think this should be added in the first patch of the series.

Fixed, thanks.

> > static bool
> > +parse_header(char *header, const char **magicp,
> > +             unsigned long int *length, uint8_t sha1[SHA1_DIGEST_SIZE])
> 
> Do you think it's worth pointing out that 'magic' is a pointer into 'header', so it shouldn't be freed?  Also, it looks to be making modifications to 'header'.

I added a header comment with details.

> > diff --git a/ovsdb/log.h b/ovsdb/log.h
> > index cc8469c01e57..4b74ca11ce6a 100644
> > --- a/ovsdb/log.h
> > +++ b/ovsdb/log.h
> > 
> > @@ -31,7 +31,7 @@ enum ovsdb_log_open_mode {
> >     OVSDB_LOG_CREATE            /* Create or open file, read/write. */
> > };
> > 
> > -#define OVSDB_MAGIC "OVSDB JSON"
> > +#define OVSDB_MAGIC "JSON"
> 
> It might be worth mentioning that "OVSDB " will be added to the definition.

I couldn't figure out exactly where this kind of file format description
should go, so I decided to just point to ovsdb(5).
diff mbox series

Patch

diff --git a/ovsdb/log.c b/ovsdb/log.c
index d25f766474dc..dd641884a2ee 100644
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -88,13 +88,19 @@  static bool rename_open_files = false;
 static bool rename_open_files = true;
 #endif
 
+static bool parse_header(char *header, const char **magicp,
+                         unsigned long int *length,
+                         uint8_t sha1[SHA1_DIGEST_SIZE]);
+static bool is_magic_ok(const char *needle, const char *haystack);
+
 /* Attempts to open 'name' with the specified 'open_mode'.  On success, stores
  * the new log into '*filep' and returns NULL; otherwise returns NULL and
  * stores NULL into '*filep'.
  *
  * 'magic' is a short text string put at the beginning of every record and used
  * to distinguish one kind of log file from another.  For a conventional OVSDB
- * log file, use OVSDB_MAGIC.
+ * log file, use OVSDB_MAGIC.  To accept more than one magic string, separate
+ * them with "|", e.g. "MAGIC 1|MAGIC 2".
  *
  * Whether the file will be locked using lockfile_lock() depends on 'locking':
  * use true to lock it, false not to lock it, or -1 to lock it only if
@@ -107,12 +113,16 @@  ovsdb_log_open(const char *name, const char *magic,
 {
     struct lockfile *lockfile;
     struct ovsdb_error *error;
-    struct ovsdb_log *file;
     struct stat s;
     FILE *stream;
     int flags;
     int fd;
 
+    /* If we can create a new file, we need to know what kind of magic to
+     * use, so there must be only one kind. */
+    if (open_mode == OVSDB_LOG_CREATE_EXCL || open_mode == OVSDB_LOG_CREATE) {
+        ovs_assert(!strchr(magic, '|'));
+    }
     *filep = NULL;
 
     ovs_assert(locking == -1 || locking == false || locking == true);
@@ -181,43 +191,54 @@  ovsdb_log_open(const char *name, const char *magic,
         goto error_unlock;
     }
 
-    if (!fstat(fd, &s)) {
-        if (s.st_size == 0) {
-            /* It's (probably) a new file so fsync() its parent directory to
-             * ensure that its directory entry is committed to disk. */
-            fsync_parent_dir(name);
-        } else if (s.st_size >= strlen(magic) && S_ISREG(s.st_mode)) {
-            /* Try to read the magic from the first log record.  If it's not
-             * the magic we expect, this is the wrong kind of file, so reject
-             * it immediately. */
-            size_t magic_len = strlen(magic);
-            char *buf = xzalloc(magic_len + 1);
-            bool err = (read(fd, buf, magic_len) == magic_len
-                        && strcmp(buf, magic));
-            free(buf);
-            if (err) {
-                error = ovsdb_error(NULL, "%s: bad magic (unexpected "
-                                    "kind of file)", name);
-                goto error_close;
-            }
-            if (lseek(fd, 0, SEEK_SET)) {
-                error = ovsdb_io_error(errno, "%s: seek failed", name);
-                goto error_close;
-            }
-        }
-    }
-
     stream = fdopen(fd, open_mode == OVSDB_LOG_READ_ONLY ? "rb" : "w+b");
     if (!stream) {
         error = ovsdb_io_error(errno, "%s: fdopen failed", name);
-        goto error_close;
+        close(fd);
+        goto error_unlock;
+    }
+
+    /* Read the magic from the first log record. */
+    char header[128];
+    const char *actual_magic;
+    if (!fgets(header, sizeof header, stream)) {
+        if (ferror(stream)) {
+            error = ovsdb_io_error(errno, "%s: read error", name);
+            goto error_fclose;
+        }
+
+        /* We need to be able to report what kind of file this is but we can't
+         * if it's empty and we accept more than one. */
+        if (strchr(magic, '|')) {
+            error = ovsdb_error(NULL, "%s: unexpected end of file", name);
+            goto error_fclose;
+        }
+        actual_magic = magic;
+
+        /* It's an empty file and therefore probably a new file, so fsync()
+         * its parent directory to ensure that its directory entry is
+         * committed to disk. */
+        fsync_parent_dir(name);
+    } else {
+        unsigned long int length;
+        uint8_t sha1[SHA1_DIGEST_SIZE];
+        if (!parse_header(header, &actual_magic, &length, sha1)
+            || !is_magic_ok(actual_magic, magic)) {
+            error = ovsdb_error(NULL, "%s: unexpected file format", name);
+            goto error_fclose;
+        }
+    }
+
+    if (fseek(stream, 0, SEEK_SET)) {
+        error = ovsdb_io_error(errno, "%s: seek failed", name);
+        goto error_fclose;
     }
 
-    file = xmalloc(sizeof *file);
+    struct ovsdb_log *file = xmalloc(sizeof *file);
     file->state = OVSDB_LOG_READ;
     file->error = NULL;
     file->name = xstrdup(name);
-    file->magic = xstrdup(magic);
+    file->magic = xstrdup(actual_magic);
     file->lockfile = lockfile;
     file->stream = stream;
     file->prev_offset = 0;
@@ -225,20 +246,43 @@  ovsdb_log_open(const char *name, const char *magic,
     *filep = file;
     return NULL;
 
-error_close:
-    close(fd);
+error_fclose:
+    fclose(stream);
 error_unlock:
     lockfile_unlock(lockfile);
 error:
     return error;
 }
 
+/* Returns true if 'needle' is one of the |-delimited words in 'haystack'. */
+static bool
+is_magic_ok(const char *needle, const char *haystack)
+{
+    /* 'needle' can't be multiple words. */
+    if (strchr(needle, '|')) {
+        return false;
+    }
+
+    size_t n = strlen(needle);
+    for (;;) {
+        if (!strncmp(needle, haystack, n) && strchr("|", haystack[n])) {
+            return true;
+        }
+        haystack = strchr(haystack, '|');
+        if (!haystack) {
+            return false;
+        }
+        haystack++;
+    }
+}
+
 void
 ovsdb_log_close(struct ovsdb_log *file)
 {
     if (file) {
         ovsdb_error_destroy(file->error);
         free(file->name);
+        free(file->magic);
         if (file->stream) {
             fclose(file->stream);
         }
@@ -247,20 +291,34 @@  ovsdb_log_close(struct ovsdb_log *file)
     }
 }
 
+const char *
+ovsdb_log_get_magic(const struct ovsdb_log *log)
+{
+    return log->magic;
+}
+
 static bool
-parse_header(const char *magic, char *header, unsigned long int *length,
-             uint8_t sha1[SHA1_DIGEST_SIZE])
+parse_header(char *header, const char **magicp,
+             unsigned long int *length, uint8_t sha1[SHA1_DIGEST_SIZE])
 {
-    char *p;
+    /* 'header' must consist of "OVSDB "... */
+    const char lead[] = "OVSDB ";
+    if (strncmp(lead, header, strlen(lead))) {
+        return false;
+    }
 
-    /* 'header' must consist of a magic string... */
-    size_t magic_len = strlen(magic);
-    if (strncmp(header, magic, magic_len) || header[magic_len] != ' ') {
+    /* ...followed by a magic string... */
+    char *magic = header + strlen(lead);
+    size_t magic_len = strcspn(magic, " ");
+    if (magic[magic_len] != ' ') {
         return false;
     }
+    magic[magic_len] = '\0';
+    *magicp = magic;
 
     /* ...followed by a length in bytes... */
-    *length = strtoul(header + magic_len + 1, &p, 10);
+    char *p;
+    *length = strtoul(magic + magic_len + 1, &p, 10);
     if (!*length || *length == ULONG_MAX || *p != ' ') {
         return false;
     }
@@ -342,7 +400,6 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
     uint8_t expected_sha1[SHA1_DIGEST_SIZE];
     uint8_t actual_sha1[SHA1_DIGEST_SIZE];
     struct ovsdb_error *error;
-    off_t data_offset;
     unsigned long data_length;
     struct json *json;
     char header[128];
@@ -356,8 +413,11 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
         error = ovsdb_io_error(errno, "%s: read failed", file->name);
         goto error;
     }
+    off_t data_offset = file->offset + strlen(header);
 
-    if (!parse_header(file->magic, header, &data_length, expected_sha1)) {
+    const char *magic;
+    if (!parse_header(header, &magic, &data_length, expected_sha1)
+        || strcmp(magic, file->magic)) {
         error = ovsdb_syntax_error(NULL, NULL, "%s: parse error at offset "
                                    "%lld in header line \"%.*s\"",
                                    file->name, (long long int) file->offset,
@@ -365,7 +425,6 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
         goto error;
     }
 
-    data_offset = file->offset + strlen(header);
     error = parse_body(file, data_offset, data_length, actual_sha1, &json);
     if (error) {
         goto error;
@@ -442,7 +501,7 @@  ovsdb_log_compose_record(const struct json *json,
     /* Compose header. */
     uint8_t sha1[SHA1_DIGEST_SIZE];
     sha1_bytes(data->string, data->length, sha1);
-    ds_put_format(header, "%s %"PRIuSIZE" "SHA1_FMT"\n",
+    ds_put_format(header, "OVSDB %s %"PRIuSIZE" "SHA1_FMT"\n",
                   magic, data->length, SHA1_ARGS(sha1));
 }
 
diff --git a/ovsdb/log.h b/ovsdb/log.h
index cc8469c01e57..4b74ca11ce6a 100644
--- a/ovsdb/log.h
+++ b/ovsdb/log.h
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,7 +31,7 @@  enum ovsdb_log_open_mode {
     OVSDB_LOG_CREATE            /* Create or open file, read/write. */
 };
 
-#define OVSDB_MAGIC "OVSDB JSON"
+#define OVSDB_MAGIC "JSON"
 
 struct ovsdb_error *ovsdb_log_open(const char *name, const char *magic,
                                    enum ovsdb_log_open_mode,
@@ -39,6 +39,8 @@  struct ovsdb_error *ovsdb_log_open(const char *name, const char *magic,
     OVS_WARN_UNUSED_RESULT;
 void ovsdb_log_close(struct ovsdb_log *);
 
+const char *ovsdb_log_get_magic(const struct ovsdb_log *);
+
 struct ovsdb_error *ovsdb_log_read(struct ovsdb_log *, struct json **)
     OVS_WARN_UNUSED_RESULT;
 void ovsdb_log_unread(struct ovsdb_log *);
diff --git a/tests/ovsdb-log.at b/tests/ovsdb-log.at
index 125ddd187551..9aa1e870039c 100644
--- a/tests/ovsdb-log.at
+++ b/tests/ovsdb-log.at
@@ -174,7 +174,7 @@  file: read: end of log
 ]], [ignore])
 AT_CHECK(
   [test-ovsdb log-io file read-only], [1], [],
-  [test-ovsdb: ovsdb error: file: bad magic (unexpected kind of file)
+  [test-ovsdb: ovsdb error: file: unexpected file format
 ])
 AT_CHECK([test -f .file.~lock~])
 AT_CLEANUP