Re: [libvirt] [PATCH 2/2] 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. * 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>]