[libvirt] [PATCHv2 2/5] storage: prepare for refactoring

Peter Krempa pkrempa at redhat.com
Fri Feb 15 21:22:32 UTC 2013


On 02/15/13 21:38, Eric Blake wrote:
> virStorageFileGetMetadataFromFD is the only caller of
> virStorageFileGetMetadataFromBuf; and it doesn't care about the
> difference between a return of 0 (total success) or 1
> (metadata was inconsistent, but pointer was populated as best
> as possible); only about a return of -1 (could not read metadata
> or out of memory).  Changing the return type, and normalizing
> the variable names used, will make merging the functions easier
> in the next commit.
>
> * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
> Change return value, and rename some variables.
> (virStorageFileGetMetadataFromFD): Rename some variables.
> ---
>   src/util/virstoragefile.c | 50 +++++++++++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index f2cbaa1..83b00e2 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -657,13 +657,15 @@ cleanup:
>   }
>
>
> -static int
> +static virStorageFileMetadataPtr
>   virStorageFileGetMetadataFromBuf(int format,
>                                    const char *path,
>                                    unsigned char *buf,
> -                                 size_t buflen,
> +                                 size_t len,
>                                    virStorageFileMetadata *meta)
>   {
> +    virStorageFileMetadata *ret = NULL;
> +
>       VIR_DEBUG("path=%s format=%d", path, format);
>
>       /* XXX we should consider moving virStorageBackendUpdateVolInfo
> @@ -671,14 +673,13 @@ virStorageFileGetMetadataFromBuf(int format,
>        */
>       if (format <= VIR_STORAGE_FILE_NONE ||
>           format >= VIR_STORAGE_FILE_LAST ||
> -        !fileTypeInfo[format].magic) {
> -        return 0;
> -    }
> +        !fileTypeInfo[format].magic)
> +        goto done;
>
>       /* Optionally extract capacity from file */
>       if (fileTypeInfo[format].sizeOffset != -1) {
> -        if ((fileTypeInfo[format].sizeOffset + 8) > buflen)
> -            return 1;
> +        if ((fileTypeInfo[format].sizeOffset + 8) > len)
> +            goto done;
>
>           if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN)
>               meta->capacity = virReadBufInt64LE(buf +
> @@ -689,7 +690,7 @@ virStorageFileGetMetadataFromBuf(int format,
>           /* Avoid unlikely, but theoretically possible overflow */
>           if (meta->capacity > (ULLONG_MAX /
>                                 fileTypeInfo[format].sizeMultiplier))
> -            return 1;
> +            goto done;
>           meta->capacity *= fileTypeInfo[format].sizeMultiplier;
>       }
>
> @@ -704,14 +705,14 @@ virStorageFileGetMetadataFromBuf(int format,
>       if (fileTypeInfo[format].getBackingStore != NULL) {
>           char *backing;
>           int backingFormat;
> -        int ret = fileTypeInfo[format].getBackingStore(&backing,
> -                                                       &backingFormat,
> -                                                       buf, buflen);
> -        if (ret == BACKING_STORE_INVALID)
> -            return 1;
> +        int store = fileTypeInfo[format].getBackingStore(&backing,
> +                                                         &backingFormat,
> +                                                         buf, len);
> +        if (store == BACKING_STORE_INVALID)
> +            goto done;
>
> -        if (ret == BACKING_STORE_ERROR)
> -            return -1;
> +        if (store == BACKING_STORE_ERROR)
> +            goto cleanup;
>
>           meta->backingStoreIsFile = false;
>           if (backing != NULL) {
> @@ -719,7 +720,7 @@ virStorageFileGetMetadataFromBuf(int format,
>               if (meta->backingStore == NULL) {
>                   virReportOOMError();
>                   VIR_FREE(backing);
> -                return -1;
> +                goto cleanup;
>               }
>               if (virBackingStoreIsFile(backing)) {
>                   meta->backingStoreIsFile = true;
> @@ -744,7 +745,10 @@ virStorageFileGetMetadataFromBuf(int format,
>           }
>       }
>
> -    return 0;
> +done:
> +    ret = meta;

Um, is the ret variable really needed here? The only value it can take 
is "meta" and return it right after. If this isn't needed in the next 
patches I'd rather go for "return meta" here.

> +cleanup:

Rename this to "error"

> +    return ret;

And "return NULL" instead;

>   }
>
>

ACK if:
1) this change will be needed later
2) tweaked according to my suggestions

Peter




More information about the libvir-list mailing list