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

Re: [libvirt] [PATCH 4/9 v2] Implement translateDiskSourcePool



On 2013年02月04日 22:03, John Ferlan wrote:
On 02/01/2013 12:24 PM, Osier Yang wrote:
It iterates over all the domain disks, and translate the source of
all the disks of 'volume' type from storage pool/volume to the real
underlying source.

Disks of type 'file', 'block', and 'dir' are supported now. Network
type is not supported yet, it will be another patch.

src/storage/storage_driver.c:
   * New helper storagePoolObjFindByDiskSource to find the specified
     pool by name.
   * Implement translateDiskSourcePool as storageTranslateDomainDiskSourcePool
---
  src/storage/storage_driver.c |  106 ++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 106 insertions(+), 0 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index e98c18c..a2c3a1a 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -47,6 +47,8 @@
  #include "virlog.h"
  #include "virfile.h"
  #include "fdstream.h"
+#include "domain_conf.h"
+#include "domain_storage.h"
  #include "configmake.h"

  #define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -2367,6 +2369,104 @@ storageListAllPools(virConnectPtr conn,
      return ret;
  }

+static virStoragePoolObjPtr
+storagePoolObjFindByDiskSource(virConnectPtr conn,
+                               const char *name)
+{
+    virStorageDriverStatePtr driver = conn->storagePrivateData;
+    virStoragePoolObjPtr pool = NULL;
+
+    storageDriverLock(driver);
+    pool = virStoragePoolObjFindByName(&driver->pools, name);
+    storageDriverUnlock(driver);
+
+    if (!pool) {
+        virReportError(VIR_ERR_NO_STORAGE_POOL,
+                       _("no storage pool with matching name '%s'"),
+                       name);
+        goto error;
+    }
+
+    if (!virStoragePoolObjIsActive(pool)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("The specified pool '%s' is not active"),
+                       name);
+        goto error;
+    }
+
+    return pool;
+error:
+    if (pool)
+        virStoragePoolObjUnlock(pool);
+    return NULL;
+}
+
+static int
+storageTranslateDomainDiskSourcePool(virConnectPtr conn,
+                                     virDomainDefPtr def)
+{
+    virStoragePoolObjPtr pool = NULL;
+    virStorageVolDefPtr vol = NULL;
+    int i;
+    int ret = -1;
+
+    for (i = 0; i<  def->ndisks; i++) {
+        virDomainDiskDefPtr disk = def->disks[i];
+
+        if (disk->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
+            continue;
+
+        if (!(pool = storagePoolObjFindByDiskSource(conn, disk->srcpool->pool)))
+            goto cleanup;
+
+        if (!(vol = virStorageVolDefFindByName(pool, disk->srcpool->volume))) {
+            virReportError(VIR_ERR_NO_STORAGE_VOL,
+                           _("no storage volume of pool '%s' with "
+                             "matching name '%s'"),
+                           disk->srcpool->pool,
+                           disk->srcpool->volume);
+            goto cleanup;
+        }
+
+        switch (vol->type) {
+        case VIR_STORAGE_VOL_FILE:
+        case VIR_STORAGE_VOL_BLOCK:
+        case VIR_STORAGE_VOL_DIR:
+            if (!vol->target.path&&
+                disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM&&
+                disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("target path of the specified volume '%s' "
+                                 "(pool = '%s') is empty"),
+                               disk->srcpool->volume
+                               disk->srcpool->pool);
+                goto cleanup;
+            }
+
+            if (vol->target.path&&
+                !(disk->src = strdup(vol->target.path))) {
+                virReportOOMError();
+                goto cleanup;
+            }

Just checking - if !vol->target.path, then disk->src will be empty, is
that expected at this point?  You've added the first check since the
initial pass at this and I want to make sure you've covered your cases.
Without comments in the code I'm not sure what is expected.

CD-ROM and Floppy disk allow the disk source be empty. So as long
as the code flows to here. It means the disk is either a CD-ROM
or a Floppy, as we checked it before. But I agreed with you that
adding a comment here will be better. Will add it when pushing.


+            break;
+        case VIR_STORAGE_VOL_NETWORK:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Using network volume as disk source is not supported"));
+            goto cleanup;
+        }
+
+        virStoragePoolObjUnlock(pool);
+        pool = NULL;
+    }
+
+    ret = 0;
+
+cleanup:
+    if (pool)
+        virStoragePoolObjUnlock(pool);
+    return ret;
+}
+
  static virStorageDriver storageDriver = {
      .name = "storage",
      .open = storageOpen, /* 0.4.0 */
@@ -2423,8 +2523,14 @@ static virStateDriver stateDriver = {
      .reload = storageDriverReload,
  };

+static virDomainStorageDriver domainStorageDriver = {
+    .translateDiskSourcePool = storageTranslateDomainDiskSourcePool,
+};
+
+
  int storageRegister(void) {
      virRegisterStorageDriver(&storageDriver);
      virRegisterStateDriver(&stateDriver);
+    virRegisterDomainStorageDriver(&domainStorageDriver);
      return 0;
  }


ACK (as long as you feel you've covered your cases for the disk storage
pool)

John

--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list


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