On 01/13/2012 11:17 AM, Peter Krempa wrote:
This patch adds a new command "desc" to show and modify notes and description for the domains using the new API.
This patch also adds a new flag for the "list" command to show notes in the domain list, to allow easy identification of VMs by storing a short description.
Example: virsh # list --note Id Name State Note ----------------------------------------------- 0 Domain-0 running Mailserver 1 2 fedora paused --- tools/virsh.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 30 +++++++- 2 files changed, 255 insertions(+), 21 deletions(-)
Looks like a great start!
+/* + * "desc" command for managing domain description and note + */ +static const vshCmdInfo info_desc[] = { + {"help", N_("show or set domain's description or note")}, + {"desc", N_("Allows to show or modify description or note of a domain.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_desc[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"persistent", VSH_OT_BOOL, 0, N_("modify persistent state of domain")}, + {"live", VSH_OT_BOOL, 0, N_("modify note only for current instance")},
Should we prefer the names --live/--config/--current, rather than just --persistent/--live, for consistency with other recently added commands?
+ {"note", VSH_OT_BOOL, 0, N_("modify the note instead of description")}, + {"edit", VSH_OT_BOOL, 0, N_("open an editor to modify the description")}, + {"new_desc", VSH_OT_ARGV, 0, N_("message")},
Underscores are pain. I'd name this new-desc. I like the slick trick to make quoting optional, where I can use either: virsh desc domain --new_desc='my description' virsh desc domain my description
+ if (live) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + if (inactive) + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (!(inactive || live)) { + flags |= VIR_DOMAIN_DESCRIPTION_CONFIG; + if (state == VIR_DOMAIN_RUNNING || state == VIR_DOMAIN_PAUSED) + flags |= VIR_DOMAIN_DESCRIPTION_LIVE; + }
This last step isn't needed; the drivers already convert _CURRENT into the correct version, so virsh doesn't have to repeat that work.
+ + if (virBufferError(&buf)) { + vshPrint(ctl, "%s", _("Failed to collect new description/note")); + goto cleanup; + } + desc = virBufferContentAndReset(&buf); + + if (edit || desc) { + if (!desc) { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0); + if (!desc) + goto cleanup; + }
This ignored --live; more on that below.
+ + if (edit) { + /* Create and open the temporary file. */ + tmp = editWriteToTempFile (ctl, desc);
Format - no space before function call (. Several instances in this function, and yes, I know it is from copy-and-paste.
+ + if (virDomainSetDescription(dom, desc, flags) < 0) { + vshError(ctl, "%s", + _("Failed to set new domain description")); + goto cleanup; + } + } else { + desc = vshGetDomainDescription(ctl, dom, note, + inactive?VIR_DOMAIN_XML_INACTIVE:0);
If the user did 'virsh desc domain --live --persistent', then you silently ignored --live; I'd almost rather have virsh error out (that is, --live and --persistent together make sense only if the user is providing a description - they don't even work together with --edit, since it is not obvious whether the editor should start from the live or from the config version).
+ if (desc) + vshPrint(ctl, "%s", desc); + else + vshPrint(ctl, _("No description for domain: %s"), + virDomainGetName(dom));
goto cleanup - this should be treated as an error condition (given that vshGetDomainDescription returns "", rather than NULL, if it successfully determined that the domain has no description).
@@ -15951,6 +16118,7 @@ static const vshCmdDef domManagementCmds[] = { {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, + {"desc", cmdDesc, opts_desc, info_desc, 0},
Sort this line earlier.
+/* extract note from domain xml */ +static char * +vshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool note, + unsigned int flags) +{ + char *desc = NULL; + char *domxml = NULL; + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL;
Here's where a getter function might make life easier (but you should still fall back to GetXMLDesc in case you are talking to older servers).
+ + /* get domains xml description and extract the note */ + if (!(domxml = virDomainGetXMLDesc(dom, flags))) { + vshError(ctl, "%s", _("Failed to retrieve domain XML")); + goto cleanup; + } + doc = virXMLParseStringCtxt(domxml, + _("(domain_definition)"), + &ctxt); + if (!doc) { + vshError(ctl, "%s", _("Couldn't parse domain XML")); + goto cleanup; + } + if (note) + desc = virXPathString("string(./description[1]/@note)", ctxt); + else + desc = virXPathString("string(./description[1])", ctxt);
If note is not present, but description is, should we fall back to a truncated version of description rather than nothing at all?
@@ -426,6 +435,25 @@ Define a domain from an XML <file>. The domain definition is registered but not started. If domain is already running, the changes will take effect on the next boot.
+=item B<desc> [I<--live> | I<--persistent>] [I<--note>] [I<--edit>] + [I<--new_desc> New description or note message] + +Show or modify description and note for a domain. These values are user +fields that allow to store arbitrary textual data to allow easy identifiaction +of domains. Note is a short (maximum 40 characters) field.
more on the characters vs. bytes distinction
+ +Flags I<--live> or I<--persistent> select wether this command works on live
s/wether/whether/
+or persistent definitions of the domain. By default both are infuenced, while
s/infuenced/influenced/
+modifying and running definition is used while reading the note.
I'm not quite sure that matches the code. That is, the code has to pass AFFECT_LIVE|AFFECT_CONFIG, and not 0, to affect both persistent and running setting. And if you switch to --live/--config/--current, that changes this paragraph even more.
+ +Flag I<--edit> specifies that an editor with the contents of current description +or note should be opened and the contents save back afterwards.
s/save/saved/
+ +Flag I<--note> selects operation on the note field instead of description. + +If neither of I<--edit> and I<--new_desc> are specified the note or description +is displayed instead of being modified. +
Overall, it sounds like a nice virsh addition. It will need some polish for v2, but shouldn't be too hard. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org