On Fri, Dec 22, 2017 at 08:13:09PM -0800, Justin Pettit wrote:
>
>
> > On Dec 7, 2017, at 4:12 PM, Ben Pfaff <[email protected]> wrote:
> >
> > diff --git a/ovsdb/log.c b/ovsdb/log.c
> > index 380f5e93d464..e6f66e668fe5 100644
> > --- a/ovsdb/log.c
> > +++ b/ovsdb/log.c
> > ...
> > @@ -53,12 +54,17 @@ struct ovsdb_log {
> > * 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.
>
> Since 'magic' is a string, it might be good to be explicit that "OVSDB_MAGIC"
> is a #define.
Thanks, I clarified the comment.
> > struct ovsdb_error *
> > -ovsdb_log_open(const char *name, enum ovsdb_log_open_mode open_mode,
> > +ovsdb_log_open(const char *name, const char *magic,
> > + enum ovsdb_log_open_mode open_mode,
> > int locking, struct ovsdb_log **filep)
> > {
> > struct lockfile *lockfile;
> > @@ -118,10 +124,30 @@ ovsdb_log_open(const char *name, enum
> > ovsdb_log_open_mode open_mode,
> > 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));
>
> It's probably hard to do much about it at this point, but if the new
> magic is a substring of existing one (e.g., just "OVSDB"), then this
> will not catch it. Probably not a big deal in practice, though.
I don't expect to introduce more than a single-digit number of magic
strings ever, so I think we can work to keep them distinct.
> > @@ -132,6 +158,7 @@ ovsdb_log_open(const char *name, enum
> > ovsdb_log_open_mode open_mode,
> >
> > file = xmalloc(sizeof *file);
> > file->name = xstrdup(name);
> > + file->magic = xstrdup(magic);
>
> I don't think this gets freed.
You are right. I fixed it.
> Acked-by: Justin Pettit <[email protected]>
Thank you for the review! I applied this to master.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
