On Fri, Oct 07, 2011 at 04:35:29PM -0600, Eric Blake wrote:
> 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>]
>
ACK, looks fine,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list