[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