[libvirt] [PATCH v2 06/15] util: Modify the FileTypeInfo to add a version size

[John Ferlan]

On 06/24/2016 09:28 AM, Peter Krempa wrote:
> On Thu, Jun 23, 2016 at 13:29:02 -0400, John Ferlan wrote:
>> The version field historically has been a 4 byte data; however, an upcoming
>> new type will use a 2 byte version.  So let's adjust for that now.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virstoragefile.c | 60 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index de4955b..37e9798 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * virstoragefile.c: file utility functions for FS storage backend
>>   *
>> - * Copyright (C) 2007-2014 Red Hat, Inc.
>> + * Copyright (C) 2007-2014, 2016 Red Hat, Inc.
>>   * Copyright (C) 2007-2008 Daniel P. Berrange
>>   *
>>   * This library is free software; you can redistribute it and/or
>> @@ -120,10 +120,12 @@ struct FileTypeInfo {
>>                           * to check at head of file */
>>      const char *extension; /* Optional file extension to check */
>>      enum lv_endian endian; /* Endianness of file format */
>> +
>>      int versionOffset;    /* Byte offset from start of file
>>                             * where we find version number,
>>                             * -1 to always fail the version test,
>>                             * -2 to always pass the version test */
>> +    int versionSize;      /* Size in bytes of version data (0, 2, or 4) */
> 
> If the size is 0 ...
> 
>>      int versionNumbers[FILE_TYPE_VERSIONS_LAST];
>>                            /* Version numbers to validate. Zeroes are ignored. */
>>      int sizeOffset;       /* Byte offset from start of file
> 
> [...]
> 
>> @@ -635,13 +637,25 @@ virStorageFileMatchesVersion(int format,
>>      if (fileTypeInfo[format].versionOffset == -2)
>>          return true;

So you would like to see:

    if (fileTypeInfo[format].versionSize == 0)
        return false;


>>  
>> -    if ((fileTypeInfo[format].versionOffset + 4) > buflen)
>> +    if ((fileTypeInfo[format].versionOffset +
>> +         fileTypeInfo[format].versionSize) > buflen)
>>          return false;
>>  
>> -    if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN)
>> -        version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset);
>> -    else
>> -        version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset);
>> +    if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
> 
> ... code below shouldn't be executed at all.
> 

It wouldn't be because for all size = 0, the versionOffset == -1 or -2
we will have returned false much sooner.

I *did* have a check at one time for size == 0, then return false, but
took it out. I can replace it though


>> +        if (fileTypeInfo[format].versionSize == 4)
>> +            version = virReadBufInt32LE(buf +
>> +                                        fileTypeInfo[format].versionOffset);
>> +        else
>> +            version = virReadBufInt16LE(buf +
>> +                                        fileTypeInfo[format].versionOffset);
>> +    } else {
>> +        if (fileTypeInfo[format].versionSize == 4)
>> +            version = virReadBufInt32BE(buf +
>> +                                        fileTypeInfo[format].versionOffset);
>> +        else
>> +            version = virReadBufInt16BE(buf +
>> +                                        fileTypeInfo[format].versionOffset);
>> +    }
> 
> ACK with the change.
>