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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list