[libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
Cole Robinson
crobinso at redhat.com
Fri Oct 11 17:54:31 UTC 2019
On 10/9/19 5:34 PM, Daniel Henrique Barboza wrote:
>
>
> On 10/7/19 6:49 PM, Cole Robinson wrote:
>> >From qemu.git docs/interop/qcow2.txt
>
> Here's the '>' again. I think this is something you're using to cite an
> external source in the commit message. Is that it?
>
>
>>
>> == String header extensions ==
>>
>> Some header extensions (such as the backing file format name and
>> the external data file name) are just a single string. In this case,
>> the header extension length is the string length and the string is
>> not '\0' terminated. (The header extension padding can make it look
>> like a string is '\0' terminated, but neither is padding always
>> necessary nor is there a guarantee that zero bytes are used
>> for padding.)
>>
>> So we shouldn't be checking for a \0 byte at the end of the backing
>> format section. I think in practice there always is a \0 but we
>> shouldn't depend on that.
>>
>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>> ---
>> src/util/virstoragefile.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 8cd576a463..5a4e4b24ae 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
>> case QCOW2_HDR_EXTENSION_END:
>> goto done;
>> - case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
>> - if (buf[offset+len] != '\0')
>> - break;
>> - *backingFormat =
>> virStorageFileFormatTypeFromString(buf+offset);
>> + case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
>> + VIR_AUTOFREE(char *) tmp = NULL;
>> + if (VIR_ALLOC_N(tmp, len + 1) < 0)
>> + return -1;
>> + memcpy(tmp, buf + offset, len);
>> + tmp[len] = '\0';
>> +
>> + *backingFormat = virStorageFileFormatTypeFromString(tmp);
>> if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>> return -1;
>> }
>> + }
>
> I suppose this extra brace is due to the new brace right after label that
> you added up there. I'm probably being a purist here, but I'd rather make
> an extra indent level in each 'case' statement of this switch just to avoid
> this braces right below each other. We have this style of switch
> statement in
> the code, so I think it would be ok.
That's my personal inclination as well, but a 'git grep -A1 "switch ("'
shows that the vast majority of switch statements don't indent the case.
The weirdness with the bracket here is just a side effect of that.
I swapped the case blocks around so at least the brackets are on
adjacent lines :)
Thanks,
Cole
More information about the libvir-list
mailing list