[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] virsh: Add support for modifying domain description and notes



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]