diff mbox series

[ovs-dev,12/13] log: Use absolute name of log file.

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

Commit Message

Ben Pfaff Dec. 8, 2017, 12:12 a.m. UTC 
In ovsdb-server, the OVSDB log code is used to open the databases specified
on the command line before ovsdb-server daemonizes itself.  Afterward, it
is occasionally necessary for ovsdb-server to reference those files by name
again.  When that happens, if daemonization changed the current directory
to the root, any relative names are no longer valid and those references
will fail.  Until now, this was handled at a higher level in ovsdb-server,
but in the future it will be convenient to handle it in the log code
itself.  This commit prepares for that by making the log code take the
absolute name of log files itself.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/log.c | 93 +++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 60 insertions(+), 33 deletions(-)

Comments

Justin Pettit Dec. 24, 2017, 12:52 a.m. UTC | #1 
> On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> @@ -487,6 +507,24 @@ ovsdb_log_unread(struct ovsdb_log *file)
>     file->offset = file->prev_offset;
> }
> 
> +static struct ovsdb_error *
> +ovsdb_log_truncate(struct ovsdb_log *file)
> +{
> +    file->state = OVSDB_LOG_WRITE;
> +
> +    struct ovsdb_error *error = NULL;
> +    if (fseeko(file->stream, file->offset, SEEK_SET)) {
> +        error = ovsdb_io_error(errno, "%s: cannot seek to offset %lld",
> +                               file->display_name,
> +                               (long long int) file->offset);
> +    } else if (ftruncate(fileno(file->stream), file->offset)) {
> +        error = ovsdb_io_error(errno, "%s: cannot truncate to length %lld",
> +                               file->display_name,
> +                               (long long int) file->offset);
> +    }
> +    return error;
> +}
> ...
> @@ -521,19 +557,13 @@ ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
>     case OVSDB_LOG_READ:
>     case OVSDB_LOG_READ_ERROR:
>     case OVSDB_LOG_WRITE_ERROR:
>         ovsdb_error_destroy(file->error);
> 
> +        file->error = ovsdb_log_truncate(file);
> +        if (file->error) {
> +            file->state = OVSDB_LOG_WRITE_ERROR;
> +            return ovsdb_error_clone(file->error);
>         }
> +        file->state = OVSDB_LOG_WRITE;
>         break;

Obviously not a big deal, but I think "file->state" will always be in OVSDB_LOG_WRITE due to the call to ovsdb_log_truncate().

> @@ -541,8 +571,7 @@ ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
> 
>     if (json->type != JSON_OBJECT && json->type != JSON_ARRAY) {
> -        error = OVSDB_BUG("bad JSON type");
> -        goto error;
> +        return OVSDB_BUG("bad JSON type");

Does this not need to set the state to OVSDB_LOG_WRITE_ERROR any more?

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

--Justin
Ben Pfaff Dec. 25, 2017, 2:22 a.m. UTC | #2 
On Sat, Dec 23, 2017 at 05:52:38PM -0700, Justin Pettit wrote:
> 
> 
> > On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > @@ -487,6 +507,24 @@ ovsdb_log_unread(struct ovsdb_log *file)
> >     file->offset = file->prev_offset;
> > }
> > 
> > +static struct ovsdb_error *
> > +ovsdb_log_truncate(struct ovsdb_log *file)
> > +{
> > +    file->state = OVSDB_LOG_WRITE;
> > +
> > +    struct ovsdb_error *error = NULL;
> > +    if (fseeko(file->stream, file->offset, SEEK_SET)) {
> > +        error = ovsdb_io_error(errno, "%s: cannot seek to offset %lld",
> > +                               file->display_name,
> > +                               (long long int) file->offset);
> > +    } else if (ftruncate(fileno(file->stream), file->offset)) {
> > +        error = ovsdb_io_error(errno, "%s: cannot truncate to length %lld",
> > +                               file->display_name,
> > +                               (long long int) file->offset);
> > +    }
> > +    return error;
> > +}
> > ...
> > @@ -521,19 +557,13 @@ ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
> >     case OVSDB_LOG_READ:
> >     case OVSDB_LOG_READ_ERROR:
> >     case OVSDB_LOG_WRITE_ERROR:
> >         ovsdb_error_destroy(file->error);
> > 
> > +        file->error = ovsdb_log_truncate(file);
> > +        if (file->error) {
> > +            file->state = OVSDB_LOG_WRITE_ERROR;
> > +            return ovsdb_error_clone(file->error);
> >         }
> > +        file->state = OVSDB_LOG_WRITE;
> >         break;
> 
> Obviously not a big deal, but I think "file->state" will always be in
> OVSDB_LOG_WRITE due to the call to ovsdb_log_truncate().

Yeah, you're right.  I didn't notice that before, but I think it's
harmless or even a good thing, so I left it.

> > @@ -541,8 +571,7 @@ ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
> > 
> >     if (json->type != JSON_OBJECT && json->type != JSON_ARRAY) {
> > -        error = OVSDB_BUG("bad JSON type");
> > -        goto error;
> > +        return OVSDB_BUG("bad JSON type");
> 
> Does this not need to set the state to OVSDB_LOG_WRITE_ERROR any more?

This seems qualitatively different to me from other write errors--it's a
bug in the caller rather than an I/O error--so I think it's OK.  Happy
to change it later if necessary.

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

Thanks, applied to master.
diff mbox series

Patch

diff --git a/ovsdb/log.c b/ovsdb/log.c
index 27f4b328d451..e364667ac673 100644
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -72,7 +72,8 @@  struct ovsdb_log {
 
     off_t prev_offset;
     off_t offset;
-    char *name;
+    char *name;                 /* Absolute name of file. */
+    char *display_name;         /* For use in log messages, etc. */
     char *magic;
     struct lockfile *lockfile;
     FILE *stream;
@@ -124,8 +125,22 @@  ovsdb_log_open(const char *name, const char *magic,
     if (open_mode == OVSDB_LOG_CREATE_EXCL || open_mode == OVSDB_LOG_CREATE) {
         ovs_assert(!strchr(magic, '|'));
     }
+
     *filep = NULL;
 
+    /* Get the absolute name of the file because we might need to access it by
+     * name again later after the process has changed directory (e.g. because
+     * daemonize() chdirs to "/").
+     *
+     * We save the user-provided name of the file for use in log messages, to
+     * reduce user confusion. */
+    char *abs_name = abs_file_name(NULL, name);
+    if (!abs_name) {
+        error = ovsdb_io_error(0, "could not determine current "
+                              "working directory");
+        goto error;
+    }
+
     ovs_assert(locking == -1 || locking == false || locking == true);
     if (locking < 0) {
         locking = open_mode != OVSDB_LOG_READ_ONLY;
@@ -238,7 +253,8 @@  ovsdb_log_open(const char *name, const char *magic,
     struct ovsdb_log *file = xmalloc(sizeof *file);
     file->state = OVSDB_LOG_READ;
     file->error = NULL;
-    file->name = xstrdup(name);
+    file->name = abs_name;
+    file->display_name = xstrdup(name);
     file->magic = xstrdup(actual_magic);
     file->lockfile = lockfile;
     file->stream = stream;
@@ -253,6 +269,7 @@  error_fclose:
 error_unlock:
     lockfile_unlock(lockfile);
 error:
+    free(abs_name);
     return error;
 }
 
@@ -284,6 +301,7 @@  ovsdb_log_close(struct ovsdb_log *file)
     if (file) {
         ovsdb_error_destroy(file->error);
         free(file->name);
+        free(file->display_name);
         free(file->magic);
         if (file->stream) {
             fclose(file->stream);
@@ -359,8 +377,9 @@  parse_body(struct ovsdb_log *file, off_t offset, unsigned long int length,
             json_parser_abort(parser);
             return ovsdb_io_error(ferror(file->stream) ? errno : EOF,
                                   "%s: error reading %lu bytes "
-                                  "starting at offset %lld", file->name,
-                                  length, (long long int) offset);
+                                  "starting at offset %lld",
+                                  file->display_name, length,
+                                  (long long int) offset);
         }
         sha1_update(&ctx, input, chunk);
         json_parser_feed(parser, input, chunk);
@@ -412,7 +431,7 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
         if (feof(file->stream)) {
             return NULL;
         }
-        error = ovsdb_io_error(errno, "%s: read failed", file->name);
+        error = ovsdb_io_error(errno, "%s: read failed", file->display_name);
         goto error;
     }
     off_t data_offset = file->offset + strlen(header);
@@ -422,7 +441,8 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
         || 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,
+                                   file->display_name,
+                                   (long long int) file->offset,
                                    (int) strcspn(header, "\n"), header);
         goto error;
     }
@@ -436,7 +456,7 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
         error = ovsdb_syntax_error(NULL, NULL, "%s: %lu bytes starting at "
                                    "offset %lld have SHA-1 hash "SHA1_FMT" "
                                    "but should have hash "SHA1_FMT,
-                                   file->name, data_length,
+                                   file->display_name, data_length,
                                    (long long int) data_offset,
                                    SHA1_ARGS(actual_sha1),
                                    SHA1_ARGS(expected_sha1));
@@ -446,7 +466,7 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
     if (json->type == JSON_STRING) {
         error = ovsdb_syntax_error(NULL, NULL, "%s: %lu bytes starting at "
                                    "offset %lld are not valid JSON (%s)",
-                                   file->name, data_length,
+                                   file->display_name, data_length,
                                    (long long int) data_offset,
                                    json->u.string);
         goto error;
@@ -454,7 +474,7 @@  ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
     if (json->type != JSON_OBJECT) {
         error = ovsdb_syntax_error(NULL, NULL, "%s: %lu bytes starting at "
                                    "offset %lld are not a JSON object",
-                                   file->name, data_length,
+                                   file->display_name, data_length,
                                    (long long int) data_offset);
         goto error;
     }
@@ -487,6 +507,24 @@  ovsdb_log_unread(struct ovsdb_log *file)
     file->offset = file->prev_offset;
 }
 
+static struct ovsdb_error *
+ovsdb_log_truncate(struct ovsdb_log *file)
+{
+    file->state = OVSDB_LOG_WRITE;
+
+    struct ovsdb_error *error = NULL;
+    if (fseeko(file->stream, file->offset, SEEK_SET)) {
+        error = ovsdb_io_error(errno, "%s: cannot seek to offset %lld",
+                               file->display_name,
+                               (long long int) file->offset);
+    } else if (ftruncate(fileno(file->stream), file->offset)) {
+        error = ovsdb_io_error(errno, "%s: cannot truncate to length %lld",
+                               file->display_name,
+                               (long long int) file->offset);
+    }
+    return error;
+}
+
 void
 ovsdb_log_compose_record(const struct json *json,
                          const char *magic, struct ds *header, struct ds *data)
@@ -512,8 +550,6 @@  ovsdb_log_compose_record(const struct json *json,
 struct ovsdb_error *
 ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
 {
-    struct ovsdb_error *error;
-
     switch (file->state) {
     case OVSDB_LOG_WRITE:
         break;
@@ -521,19 +557,13 @@  ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
     case OVSDB_LOG_READ:
     case OVSDB_LOG_READ_ERROR:
     case OVSDB_LOG_WRITE_ERROR:
-        file->state = OVSDB_LOG_WRITE;
         ovsdb_error_destroy(file->error);
-        file->error = NULL;
-        if (fseeko(file->stream, file->offset, SEEK_SET)) {
-            error = ovsdb_io_error(errno, "%s: cannot seek to offset %lld",
-                                   file->name, (long long int) file->offset);
-            goto error;
-        }
-        if (ftruncate(fileno(file->stream), file->offset)) {
-            error = ovsdb_io_error(errno, "%s: cannot truncate to length %lld",
-                                   file->name, (long long int) file->offset);
-            goto error;
+        file->error = ovsdb_log_truncate(file);
+        if (file->error) {
+            file->state = OVSDB_LOG_WRITE_ERROR;
+            return ovsdb_error_clone(file->error);
         }
+        file->state = OVSDB_LOG_WRITE;
         break;
 
     case OVSDB_LOG_BROKEN:
@@ -541,8 +571,7 @@  ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
     }
 
     if (json->type != JSON_OBJECT && json->type != JSON_ARRAY) {
-        error = OVSDB_BUG("bad JSON type");
-        goto error;
+        return OVSDB_BUG("bad JSON type");
     }
 
     struct ds header = DS_EMPTY_INITIALIZER;
@@ -557,33 +586,31 @@  ovsdb_log_write(struct ovsdb_log *file, const struct json *json)
     ds_destroy(&header);
     ds_destroy(&data);
     if (!ok) {
-        error = ovsdb_io_error(errno, "%s: write failed", file->name);
+        int error = errno;
 
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
         VLOG_WARN_RL(&rl, "%s: write failed (%s)",
-                     file->name, ovs_strerror(errno));
+                     file->name, ovs_strerror(error));
 
         /* Remove any partially written data, ignoring errors since there is
          * nothing further we can do. */
         ignore(ftruncate(fileno(file->stream), file->offset));
 
-        goto error;
+        file->error = ovsdb_io_error(error, "%s: write failed",
+                                     file->display_name);
+        file->state = OVSDB_LOG_WRITE_ERROR;
+        return ovsdb_error_clone(file->error);
     }
 
     file->offset += total_length;
     return NULL;
-
-error:
-    file->state = OVSDB_LOG_WRITE_ERROR;
-    file->error = ovsdb_error_clone(error);
-    return error;
 }
 
 struct ovsdb_error *
 ovsdb_log_commit(struct ovsdb_log *file)
 {
     if (file->stream && fsync(fileno(file->stream))) {
-        return ovsdb_io_error(errno, "%s: fsync failed", file->name);
+        return ovsdb_io_error(errno, "%s: fsync failed", file->display_name);
     }
     return NULL;
 }