[libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check

[Daniel Henrique Barboza]

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.


DHB


>   
>           offset += len;
>       }