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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list