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

Re: [libvirt] [PATCH 34/26] snapshot: add <disks> to snapshot xml



On 08/19/2011 03:58 PM, Eric Blake wrote:
Adds an optional element to<domainsnapshot>, which will be used
to give user control over external snapshot filenames on input,
and specify generated filenames on output.

+    /* Double check requested disks.  */
+    for (i = 0; i<  def->ndisks; i++) {
+        virDomainSnapshotDiskDefPtr disk =&def->disks[i];
+        bool found = false;
+
+        for (j = 0; j<  def->dom->ndisks; j++) {
+            if (STREQ(disk->name, def->dom->disks[j]->dst)) {
+                int disk_snapshot = def->dom->disks[j]->snapshot;

Rather than open-code my own name lookup, I just noticed virDomainDiskIndexByName. And using that function has another benefit, to come up in my next path - 'virsh domblkinfo domain disk-name' should accept the same set of names as 'virsh snapshot-create' xml (right now, they don't - domblkinfo uses the source path instead of the target device string; what's worse is the source path is not necessarily unique). Squash this in:

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 50a9bb4..043e73f 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -11218,7 +11218,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
     int ret = -1;
     virBitmapPtr map = NULL;
     int i;
-    int j;
     bool inuse;

     if (!def->dom) {
@@ -11247,46 +11246,42 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
     /* Double check requested disks.  */
     for (i = 0; i < def->ndisks; i++) {
         virDomainSnapshotDiskDefPtr disk = &def->disks[i];
-        bool found = false;
+        int idx = virDomainDiskIndexByName(def->dom, disk->name);
+        int disk_snapshot;

-        for (j = 0; j < def->dom->ndisks; j++) {
-            if (STREQ(disk->name, def->dom->disks[j]->dst)) {
-                int disk_snapshot = def->dom->disks[j]->snapshot;
+        if (idx < 0) {
+            virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                 _("no disk named '%s'"), disk->name);
+            goto cleanup;
+        }
+        disk_snapshot = def->dom->disks[idx]->snapshot;

-                if (virBitmapGetBit(map, j, &inuse) < 0 || inuse) {
-                    virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                         _("disk '%s' specified twice"),
-                                         disk->name);
-                    goto cleanup;
-                }
-                ignore_value(virBitmapSetBit(map, j));
-                disk->index = j;
-                if (!disk_snapshot)
-                    disk_snapshot = default_snapshot;
-                if (!disk->snapshot) {
-                    disk->snapshot = disk_snapshot;
-                } else if (disk_snapshot && require_match &&
-                           disk->snapshot != disk_snapshot) {
-                    virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk '%s' must use snapshot mode "
-                                           "'%s'"), disk->name,
- virDomainDiskSnapshotTypeToString(disk_snapshot));
-                    goto cleanup;
-                }
-                if (disk->file &&
-                    disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) {
-                    virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("file '%s' for disk '%s' requires " - "use of external snapshot mode"),
-                                         disk->file, disk->name);
-                    goto cleanup;
-                }
-                break;
-            }
+        if (virBitmapGetBit(map, idx, &inuse) < 0 || inuse) {
+            virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                 _("disk '%s' specified twice"),
+                                 disk->name);
+            goto cleanup;
         }
-        if (!found) {
+        ignore_value(virBitmapSetBit(map, idx));
+        disk->index = idx;
+        if (!disk_snapshot)
+            disk_snapshot = default_snapshot;
+        if (!disk->snapshot) {
+            disk->snapshot = disk_snapshot;
+        } else if (disk_snapshot && require_match &&
+                   disk->snapshot != disk_snapshot) {
+ const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot);
             virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                 _("no disk named '%s'"), disk->name);
+ _("disk '%s' must use snapshot mode '%s'"),
+                                 disk->name, tmp);
+            goto cleanup;
+        }
+        if (disk->file &&
+            disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) {
+            virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                 _("file '%s' for disk '%s' requires "
+                                   "use of external snapshot mode"),
+                                 disk->file, disk->name);
             goto cleanup;
         }
     }

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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