snapshot: avoid accidental renames with snapshot-edit

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.

(**) Renaming or creating a clone is still a risky proposition -
for offline snapshots and system checkpoints, if the new name
does not match an actual name recorded in the qcow2 internal
snapshots, then you cannot revert to the new checkpoint.  But it
is assumed that anyone using the new virsh flags knows what they
are doing, and can deal with the fallout caused by a rename/clone;
that is, we can't completely prevent a user from shooting
themselves in the foot, so much as we are making the default
action less risky.

* tools/virsh.c (cmdSnapshotEdit): Add --rename, --clone.
* tools/virsh.pod (snapshot-edit): Document them.

diff --git a/tools/virsh.c b/tools/virsh.c
index 20b3dc5a61bfca606dc358bdb33b3adf1b40d365..7151694a231c705baf3c67c2d4e857cbe50c7a21 100644 (file)
--- 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 1f7c76f3d2ea7778eca820533e219c7930dea700..8ce41fb5d48c0a815f61fc5a844198fd6c4ee2d3 100644 (file)
--- 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,14 @@ 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 clone of the snapshot metadata.  If neither is specified,
+then the edits must not change the snapshot name.  Note that changing
+a snapshot name must be done with care, since the contents of some
+snapshots, such as internal snapshots within a single qcow2 file, are
+accessible only from the original name.
+
 =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}]
 [I<--metadata>]