[libvirt] [PATCH 3/4] storage: refactor metadata lookup

Eric Blake eblake at redhat.com
Thu Feb 7 05:10:19 UTC 2013


Prior to this patch, we had the callchains:
external users
  \-> virStorageFileGetMetadataFromFD
      \-> virStorageFileGetMetadataFromBuf
virStorageFileGetMetadataRecurse
  \-> virStorageFileGetMetadataFromFD
      \-> virStorageFileGetMetadataFromBuf

However, a future patch wants to add an additional parameter to
the bottom of the chain, for use by virStorageFileGetMetadataRecurse,
without affecting existing external callers.  Since there is only a
single caller of the internal function, we can repurpose it to fit
our needs, with this patch giving us:

external users
  \-> virStorageFileGetMetadataFromFD
      \-> virStorageFileGetMetadataInternal
virStorageFileGetMetadataRecurse /
  \-> virStorageFileGetMetadataInternal

Note that no callers cared whether virStorageFileGetMetadataFromBuf
returned 0 (success) or 1 (able to read, but not able to decipher),
only whether there was failure (not able to read).

* src/util/virstoragefile.c (virStorageFileGetMetadataFromFD):
Move most of the guts...
(virStorageFileGetMetadataFromBuf): ...here, and rename...
(virStorageFileGetMetadataInternal): ...to this.
(virStorageFileGetMetadataRecurse): Use internal helper.
---
 src/util/virstoragefile.c | 156 ++++++++++++++++++++++------------------------
 1 file changed, 75 insertions(+), 81 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index cfd424d..1a4557e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -708,28 +708,70 @@ cleanup:
 }


-static int
-virStorageFileGetMetadataFromBuf(int format,
-                                 const char *path,
-                                 unsigned char *buf,
-                                 size_t buflen,
-                                 virStorageFileMetadata *meta)
+static virStorageFileMetadataPtr
+virStorageFileGetMetadataInternal(const char *path,
+                                  int fd,
+                                  int format)
 {
-    VIR_DEBUG("path=%s format=%d", path, format);
+    virStorageFileMetadata *meta = NULL;
+    unsigned char *buf = NULL;
+    ssize_t len = STORAGE_MAX_HEAD;
+    virStorageFileMetadata *ret = NULL;
+    struct stat sb;
+
+    VIR_DEBUG("path=%s, fd=%d, format=%d", path, fd, format);
+
+    if (VIR_ALLOC(meta) < 0) {
+        virReportOOMError();
+        return NULL;
+    }
+
+    if (fstat(fd, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("cannot stat file '%s'"),
+                             path);
+        goto cleanup;
+    }
+
+    /* No header to probe for directories, but also no backing file */
+    if (S_ISDIR(sb.st_mode))
+        return meta;
+
+    if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
+        virReportSystemError(errno, _("cannot seek to start of '%s'"), path);
+        goto cleanup;
+    }
+
+    if (VIR_ALLOC_N(buf, len) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if ((len = read(fd, buf, len)) < 0) {
+        virReportSystemError(errno, _("cannot read header '%s'"), path);
+        goto cleanup;
+    }
+
+    if (format == VIR_STORAGE_FILE_AUTO)
+        format = virStorageFileProbeFormatFromBuf(path, buf, len);
+
+    if (format <= VIR_STORAGE_FILE_NONE ||
+        format >= VIR_STORAGE_FILE_LAST) {
+        virReportSystemError(EINVAL, _("unknown storage file format %d"),
+                             format);
+        goto cleanup;
+    }

     /* XXX we should consider moving virStorageBackendUpdateVolInfo
      * code into this method, for non-magic files
      */
-    if (format <= VIR_STORAGE_FILE_NONE ||
-        format >= VIR_STORAGE_FILE_LAST ||
-        !fileTypeInfo[format].magic) {
-        return 0;
-    }
+    if (!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 = virRead64LE(buf +
@@ -740,7 +782,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;
     }

@@ -754,14 +796,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) {
@@ -769,7 +811,7 @@ virStorageFileGetMetadataFromBuf(int format,
             if (meta->backingStore == NULL) {
                 virReportOOMError();
                 VIR_FREE(backing);
-                return -1;
+                goto cleanup;
             }
             if (virBackingStoreIsFile(backing)) {
                 meta->backingStoreIsFile = true;
@@ -794,7 +836,14 @@ virStorageFileGetMetadataFromBuf(int format,
         }
     }

-    return 0;
+done:
+    ret = meta;
+    meta = NULL;
+
+cleanup:
+    virStorageFileFreeMetadata(meta);
+    VIR_FREE(buf);
+    return ret;
 }


@@ -907,62 +956,7 @@ virStorageFileGetMetadataFromFD(const char *path,
                                 int fd,
                                 int format)
 {
-    virStorageFileMetadata *meta = NULL;
-    unsigned char *head = NULL;
-    ssize_t len = STORAGE_MAX_HEAD;
-    virStorageFileMetadata *ret = NULL;
-    struct stat sb;
-
-    if (VIR_ALLOC(meta) < 0) {
-        virReportOOMError();
-        return NULL;
-    }
-
-    if (fstat(fd, &sb) < 0) {
-        virReportSystemError(errno,
-                             _("cannot stat file '%s'"),
-                             path);
-        goto cleanup;
-    }
-
-    /* No header to probe for directories, but also no backing file */
-    if (S_ISDIR(sb.st_mode))
-        return meta;
-
-    if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
-        virReportSystemError(errno, _("cannot seek to start of '%s'"), path);
-        goto cleanup;
-    }
-
-    if (VIR_ALLOC_N(head, len) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
-    if ((len = read(fd, head, len)) < 0) {
-        virReportSystemError(errno, _("cannot read header '%s'"), path);
-        goto cleanup;
-    }
-
-    if (format == VIR_STORAGE_FILE_AUTO)
-        format = virStorageFileProbeFormatFromBuf(path, head, len);
-
-    if (format <= VIR_STORAGE_FILE_NONE ||
-        format >= VIR_STORAGE_FILE_LAST) {
-        virReportSystemError(EINVAL, _("unknown storage file format %d"),
-                             format);
-        goto cleanup;
-    }
-
-    if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0)
-        goto cleanup;
-    ret = meta;
-    meta = NULL;
-
-cleanup:
-    virStorageFileFreeMetadata(meta);
-    VIR_FREE(head);
-    return ret;
+    return virStorageFileGetMetadataInternal(path, fd, format);
 }

 /* Recursive workhorse for virStorageFileGetMetadata.  */
@@ -991,7 +985,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format,
         return NULL;
     }

-    ret = virStorageFileGetMetadataFromFD(path, fd, format);
+    ret = virStorageFileGetMetadataInternal(path, fd, format);

     if (VIR_CLOSE(fd) < 0)
         VIR_WARN("could not close file %s", path);
-- 
1.8.1




More information about the libvir-list mailing list