[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]