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

Ben Pfaff blp at ovn.org
Sun Dec 24 21:58:44 UTC 2017


On Sat, Dec 23, 2017 at 04:41:50PM -0700, Justin Pettit wrote:
> 
> > On Dec 7, 2017, at 5:12 PM, Ben Pfaff <blp at 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).


More information about the dev mailing list