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

Justin Pettit jpettit at ovn.org
Sun Dec 24 00:11:52 UTC 2017


I forgot to add:

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


> On Dec 23, 2017, at 4:41 PM, Justin Pettit <jpettit at ovn.org> 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.
> 
>> 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
> 
> 



More information about the dev mailing list