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

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/