[PATCH 2/2] snapshot: avoid accidental renames with snapshot-edit

[Eric Blake <eblake@xxxxxxxxxx>]

I was a bit surprised that 'virsh snapshot-edit dom name' silently
allowed me to clone things, while still telling me the old name,
especially since other commands like 'virsh edit dom' reject rename
attempts (*).  This fixes things to be more explicit.

(*) Technically, 'virsh edit dom' relies on virDomainDefineXML
behavior, which rejects attempts to mix a new name with existing
uuid or new uuid with existing name, but you can create a new
domain by changing both uuid and name.  On the other hand, while
snapshot-edit --clone is a true clone, creating a new domain
would also have to decide whether to clone snapshot metadata,
managed save, and any other secondary data related to the domain.
Domain renames are not trivial either.

* tools/virsh.c (cmdSnapshotEdit): Add --rename, --clone.
* tools/virsh.pod (snapshot-edit): Document them.
---
 tools/virsh.c   |   45 ++++++++++++++++++++++++++++++++++++++++-----
 tools/virsh.pod |    6 ++++++
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 20b3dc5..7151694 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -12830,6 +12830,8 @@ static const vshCmdOptDef opts_snapshot_edit[] = {
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
     {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")},
     {"current", VSH_OT_BOOL, 0, N_("also set edited snapshot as current")},
+    {"rename", VSH_OT_BOOL, 0, N_("allow renaming an existing snapshot")},
+    {"clone", VSH_OT_BOOL, 0, N_("allow cloning to new name")},
     {NULL, 0, 0, NULL}
 };

@@ -12838,13 +12840,23 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom = NULL;
     virDomainSnapshotPtr snapshot = NULL;
+    virDomainSnapshotPtr edited = NULL;
     const char *name;
+    const char *edited_name;
     bool ret = false;
     char *tmp = NULL;
     char *doc = NULL;
     char *doc_edited = NULL;
     unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE;
     unsigned int define_flags = VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
+    bool rename_okay = vshCommandOptBool(cmd, "rename");
+    bool clone_okay = vshCommandOptBool(cmd, "clone");
+
+    if (rename_okay && clone_okay) {
+        vshError(ctl, "%s",
+                 _("--rename and --clone are mutually exclusive"));
+        return false;
+    }

     if (vshCommandOptBool(cmd, "current"))
         define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;
@@ -12867,8 +12879,6 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
     doc = virDomainSnapshotGetXMLDesc(snapshot, getxml_flags);
     if (!doc)
         goto cleanup;
-    virDomainSnapshotFree(snapshot);
-    snapshot = NULL;

     /* strstr is safe here, since xml came from libvirt API and not user */
     if (strstr(doc, "<state>disk-snapshot</state>"))
@@ -12899,13 +12909,36 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
     }

     /* Everything checks out, so redefine the xml.  */
-    snapshot = virDomainSnapshotCreateXML(dom, doc_edited, define_flags);
-    if (!snapshot) {
+    edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags);
+    if (!edited) {
         vshError(ctl, _("Failed to update %s"), name);
         goto cleanup;
     }

-    vshPrint(ctl, _("Snapshot %s edited.\n"), name);
+    edited_name = virDomainSnapshotGetName(edited);
+    if (STREQ(name, edited_name)) {
+        vshPrint(ctl, _("Snapshot %s edited.\n"), name);
+    } else if (clone_okay) {
+        vshPrint(ctl, _("Snapshot %s cloned to %s.\n"), name,
+                 edited_name);
+    } else {
+        unsigned int delete_flags;
+
+        delete_flags = VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY;
+        if (virDomainSnapshotDelete(rename_okay ? snapshot : edited,
+                                    delete_flags) < 0) {
+            virshReportError(ctl);
+            vshError(ctl, _("Failed to clean up %s"),
+                     rename_okay ? name : edited_name);
+            goto cleanup;
+        }
+        if (!rename_okay) {
+            vshError(ctl, _("Must use --rename or --clone to change %s to %s"),
+                     name, edited_name);
+            goto cleanup;
+        }
+    }
+
     ret = true;

 cleanup:
@@ -12917,6 +12950,8 @@ cleanup:
     }
     if (snapshot)
         virDomainSnapshotFree(snapshot);
+    if (edited)
+        virDomainSnapshotFree(edited);
     if (dom)
         virDomainFree(dom);
     return ret;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 1f7c76f..3351fe0 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1952,6 +1952,7 @@ With I<snapshotname>, this is a request to make the existing named
 snapshot become the current snapshot, without reverting the domain.

 =item B<snapshot-edit> I<domain> I<snapshotname> [I<--current>]
+{[I<--rename>] | [I<--clone>]}

 Edit the XML configuration file for I<snapshotname> of a domain.  If
 I<--current> is specified, also force the edited snapshot to become
@@ -1968,6 +1969,11 @@ except that it does some error checking.
 The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment
 variables, and defaults to C<vi>.

+If I<--rename> is specified, then the edits can change the snapshot
+name.  If I<--clone> is specified, then changing the snapshot name
+will create a cloned snapshot.  If neither is specified, then the
+edits must not change the snapshot name.
+
 =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}]
 [I<--metadata>]

-- 
1.7.4.4

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