[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