[libvirt] [PATCH] virsh: Add --name and --description options to snapshot-create
This options are shortcuts to set name and description of a snapshot. Suggested by Elias Probst --- I'm not sure if this is be best approach. In case of the vol-* commands there is vol-create that takes and XML file and vol-create-as that takes a set of arguments. So, should there actually be a snapshot-create-as to takes --name and --description options? Matthias tools/virsh.c | 39 ++++++++++++++++++++++++++++++++++++--- tools/virsh.pod | 6 ++++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2e35021..2ddb2f7 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10214,6 +10214,8 @@ static const vshCmdInfo info_snapshot_create[] = { static const vshCmdOptDef opts_snapshot_create[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"xmlfile", VSH_OT_DATA, 0, N_("domain snapshot XML")}, + {"name", VSH_OT_DATA, 0, N_("snapshot name")}, + {"description", VSH_OT_DATA, 0, N_("snapshot description")}, {NULL, 0, 0, NULL} }; @@ -10223,6 +10225,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; int ret = FALSE; const char *from = NULL; + const char *suggested_name = NULL; + const char *description = NULL; char *buffer = NULL; virDomainSnapshotPtr snapshot = NULL; xmlDocPtr xml = NULL; @@ -10237,9 +10241,38 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "xmlfile", &from) <= 0) - buffer = vshStrdup(ctl, "<domainsnapshot/>"); - else { + if (vshCommandOptString(cmd, "xmlfile", &from) <= 0) { + if (vshCommandOptString(cmd, "name", &suggested_name) < 0 || + vshCommandOptString(cmd, "description", &description) < 0) { + goto cleanup; + } + + if (suggested_name != NULL || description != NULL) { + virBuffer tmp = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&tmp, "<domainsnapshot>\n"); + + if (suggested_name != NULL) + virBufferEscapeString(&tmp, " <name>%s</name>\n", + suggested_name); + + if (description != NULL) + virBufferEscapeString(&tmp, " <description>%s</description>\n", + description); + + virBufferAddLit(&tmp, "</domainsnapshot>"); + + if (virBufferError(&tmp)) { + virBufferFreeAndReset(&tmp); + vshPrint(ctl, "%s", _("Failed to allocate XML buffer")); + goto cleanup; + } + + buffer = virBufferContentAndReset(&tmp); + } else { + buffer = vshStrdup(ctl, "<domainsnapshot/>"); + } + } else { if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { /* we have to report the error here because during cleanup * we'll run through virDomainFree(), which loses the diff --git a/tools/virsh.pod b/tools/virsh.pod index 9c42008..aa238d9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1223,12 +1223,14 @@ used to represent properties of snapshots. =over 4 -=item B<snapshot-create> I<domain> I<xmlfile> +=item B<snapshot-create> I<domain> I<xmlfile> optional I<name> I<description> Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. The only properties settable for a domain snapshot are the <name> and <description>; the rest of the fields are ignored, and -automatically filled in by libvirt. If I<xmlfile> is completely omitted, +automatically filled in by libvirt. The values for <name> and <description> +can also be specified using I<name> and I<description> when I<xmlfile> is +ommited. If I<xmlfile>, I<name> and I<description> are completely omitted, then libvirt will choose a value for all fields. =item B<snapshot-current> I<domain> -- 1.7.0.4
On 04/10/2011 05:31 AM, Matthias Bolte wrote:
This options are shortcuts to set name and description of a snapshot.
Suggested by Elias Probst ---
I'm not sure if this is be best approach. In case of the vol-* commands there is vol-create that takes and XML file and vol-create-as that takes a set of arguments.
So, should there actually be a snapshot-create-as to takes --name and --description options?
For consistency, I would argue that we don't have any other commands that modify direct xml input, and so we shouldn't be starting that now. I agree that a snapshot-create-as that takes options (and builds xml under the hood) would match with existing virsh paradigms better than making the existing snapshot-create command modify input xml on the fly. In other words, I'm afraid that your concern was unfortunately valid, and that we should NACK this patch in favor of one that adds a new command instead. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Apr 11, 2011 at 11:36:00AM -0600, Eric Blake wrote:
On 04/10/2011 05:31 AM, Matthias Bolte wrote:
This options are shortcuts to set name and description of a snapshot.
Suggested by Elias Probst ---
I'm not sure if this is be best approach. In case of the vol-* commands there is vol-create that takes and XML file and vol-create-as that takes a set of arguments.
So, should there actually be a snapshot-create-as to takes --name and --description options?
For consistency, I would argue that we don't have any other commands that modify direct xml input, and so we shouldn't be starting that now. I agree that a snapshot-create-as that takes options (and builds xml under the hood) would match with existing virsh paradigms better than making the existing snapshot-create command modify input xml on the fly. In other words, I'm afraid that your concern was unfortunately valid, and that we should NACK this patch in favor of one that adds a new command instead.
Agreed ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte