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

Re: [libvirt] [PATCHv3 3/4] virsh: Add support for modifying domain description and titles



On 02/01/2012 06:03 AM, Peter Krempa wrote:
> This patch adds a new command "desc" to show and modify titles and
> description for the domains using the new API.
> 
> This patch also adds a new flag for the "list" command to show titles in
> the domain list, to allow easy identification of VMs by storing a short
> description.
> 
> Example:
> virsh # list --title
>  Id Name                 State      Title
>  -----------------------------------------------
>    0 Domain-0             running    Mailserver 1
>    2 fedora               paused
> ---
>  tools/virsh.c   |  283 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  tools/virsh.pod |   34 +++++++-
>  2 files changed, 296 insertions(+), 21 deletions(-)
> 

> +static bool
> +cmdDesc(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> +{
> +    virDomainPtr dom;
> +    bool config = vshCommandOptBool(cmd, "config");
> +    bool live = vshCommandOptBool(cmd, "live");
> +    /* current is ignored */

We could copy some other commands that make sure --current is
mutually-exclusive with either --config or --live; but I'm not too
worried about that.

> +
> +    bool title = vshCommandOptBool(cmd, "title");
> +    bool edit = vshCommandOptBool(cmd, "edit");
> +
> +    int state;
> +    int type;
> +    char *desc = NULL;
> +    char *desc_edited = NULL;
> +    char *tmp = NULL;
> +    char *tmpstr;
> +    const vshCmdOpt *opt = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    bool pad = false;
> +    bool ret = false;
> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn))
> +        return false;
> +
> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if ((state = vshDomainState(ctl, dom, NULL)) < 0) {
> +        ret = false;

Redundant assignment, since ret started false.

> +        /* strip a possible newline at the end of file */
> +        /* some editors enforce a newline, this makes editing the title
> +         * more convinient */

s/convinient/convenient/

> +        if (title &&
> +            (tmpstr = strrchr(desc, '\n')) &&
> +            *(tmpstr+1) == '\0')
> +            *tmpstr = '\0';
> +
> +        if (virDomainSetMetadata(dom, type, desc, NULL, NULL, flags) < 0) {
> +            vshError(ctl, "%s",
> +                     _("Failed to set new domain description"));
> +            goto cleanup;
> +        }
> +        vshPrint(ctl, "%s", _("Domain description updated successfuly"));

s/successfuly/successfully/

> +    } else {
> +        desc = vshGetDomainDescription(ctl, dom, title,
> +                                       config?VIR_DOMAIN_XML_INACTIVE:0);
> +        if (!desc)
> +            goto cleanup;
> +
> +        if (strlen(desc) > 0)
> +            vshPrint(ctl, "%s", desc);
> +        else
> +            vshPrint(ctl, _("No description for domain: %s"),
> +                     virDomainGetName(dom));

This is, of course, ambiguous output (since I can give my domain the
<description>No description for domain: self</description>).  Perhaps we
should consider returning false when there is no description?  But the
ambiguity is a corner case, so I'm okay if you don't change anything here.

> +        if (err && err->code == VIR_ERR_NO_DOMAIN_METADATA) {
> +            desc = vshStrdup(ctl, "");
> +            virResetLastError();
> +            return desc;
> +        }

Nice - makes it slightly easier to use throughout the rest of virsh.

> +
> +        if (err &&  err->code != VIR_ERR_NO_SUPPORT)

Is the double space intentional?

> +            return desc;
> +    }
> +
> +    /* fall back to xml */
> +    /* get domains xml description and extract the title/description */

s/domains/domain's/

> +=item B<desc> [I<--live> | I<--config>] [I<--title>] [I<--edit>]
> +              [I<--new-desc> New description or title message]

This didn't document --current; then again, since you ignore it on
parsing, I don't quite care (but you've been warned if QE files a bug
report about inconsistency in the future :)

> +
> +Show or modify description and title of a domain. These values are user
> +fields that allow to store arbitrary textual data to allow easy identifiaction

s/identifiaction/identification/

> +of domains. Note should be short, although it's not enforced.
> +
> +Flags I<--live> or I<--config> 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.
> +
> +If both I<--live> and I<--config> are specified, the I<--config> option takes
> +predcedence on getting the current description and both live configuration

s/predcedence/precedence/

> +and config are updated while setting the description.
> +
> +Flag I<--edit> specifies that an editor with the contents of current description
> +or note should be opened and the contents saved back afterwards.
> +
> +Flag I<--title> 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.

ACK with nits fixed.

-- 
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]