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

[Eric Blake <eblake@xxxxxxxxxx>]

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@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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