I forgot to add: Acked-by: Justin Pettit <[email protected]>
> On Dec 23, 2017, at 4:41 PM, Justin Pettit <[email protected]> wrote: > > >> On Dec 7, 2017, at 5:12 PM, Ben Pfaff <[email protected]> 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
