On Sat, Sep 07, 2024 at 17:15:21 +0300, Nikolai Barybin via Devel wrote:
> Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> ---
> src/storage_file/storage_file_probe.c | 53 +++++++++++++++++++++++++--
> 1 file changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c
> index 4792b9fdff..21430d18aa 100644
> --- a/src/storage_file/storage_file_probe.c
> +++ b/src/storage_file/storage_file_probe.c
> @@ -106,6 +106,7 @@ qcow2GetClusterSize(const char *buf,
> size_t buf_size);
> static int qcowXGetBackingStore(char **, int *,
> const char *, size_t);
> +static int qcowXGetDataFile(char **, char *, int, size_t);
data file is a qcow2 only feature so calling this 'qcowX' makes no sense
as it won't be reused.
> static int qcow2GetFeatures(virBitmap **features, int format,
> char *buf, ssize_t len);
> static int vmdk4GetBackingStore(char **, int *,
> @@ -393,7 +395,8 @@ cowGetBackingStore(char **res,
> static int
> qcow2GetExtensions(const char *buf,
> size_t buf_size,
> - int *backingFormat)
> + int *backingFormat,
> + char **dataFilePath)
> {
> size_t offset;
> size_t extension_start;
> @@ -488,6 +491,17 @@ qcow2GetExtensions(const char *buf,
> break;
> }
>
> + case QCOW2_HDR_EXTENSION_DATA_FILE_NAME: {
> + g_autofree char *tmp = NULL;
'tmp' is unused.
> + if (!dataFilePath)
> + break;
> +
> + *(char **)dataFilePath = g_new0(char, len + 1);
No need for that amount of typecasts since 'dataFilePath' is aready 'char
**'
> + memcpy(*(char **)dataFilePath, buf + offset, len);
same here.
> + (*((char **)dataFilePath))[len] = '\0';
This is not needed as g_new0 allocates a zeroed-out buffer.
> + break;
> + }
> +
> case QCOW2_HDR_EXTENSION_END:
> return 0;
> }
> @@ -554,9 +568,34 @@ qcowXGetBackingStore(char **res,
> memcpy(*res, buf + offset, size);
> (*res)[size] = '\0';
>
> - if (qcow2GetExtensions(buf, buf_size, format) < 0)
> + if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0)
> + return 0;
> +
> + return 0;
> +}
> +
> +
> +static int
> +qcowXGetDataFile(char **res,
> + char *buf,
> + int format,
> + size_t buf_size)
> +{
> + virBitmap *features = NULL;
> + char *filename = NULL;
> + *res = NULL;
> +
> + if (buf_size < QCOW2v3_HDR_FEATURES_INCOMPATIBLE + 8)
> return 0;
>
> + ignore_value(qcow2GetFeatures(&features, format, buf, buf_size));
'qcowXGetDataFile' gets called one block before 'qcow2GetFeatures' would
be called via
'fileTypeInfo[meta->format].getFeatures(&meta->features,'
but here you do it again. Restrucutre the code so that iut doesn't need
to do this twice.
> + if (features && virBitmapIsBitSet(features, VIR_STORAGE_FILE_FEATURE_DATA_FILE)) {
> + if (qcow2GetExtensions(buf, buf_size, NULL, &filename) < 0)
So 'filename' will be filled with an allocated buffer ...
> + return 0;
> +
> + *res = g_strdup(filename);
... you'll dup it (which is wasteful) and then leak the original.
> + }
> +
> return 0;
> }
>
> @@ -963,6 +1002,12 @@ virStorageFileProbeGetMetadata(virStorageSource *meta,
> meta->backingStoreRawFormat = format;
> }
>
> + VIR_FREE(meta->dataFileRaw);
> + if (fileTypeInfo[meta->format].getDataFile != NULL) {
> + fileTypeInfo[meta->format].getDataFile(&meta->dataFileRaw,
> + buf, meta->format, len);
> + }
> +
> g_clear_pointer(&meta->features, virBitmapFree);
> if (fileTypeInfo[meta->format].getFeatures != NULL &&
> fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0)
> --
> 2.43.5
>
