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

Re: [libvirt] [PATCH] virsh: Enhance list command to ease creation of shell scripts



On 02/21/2012 09:05 AM, Peter Krempa wrote:
> This patch adds new options to the "virsh list" command enabling
> filtering of persistent and transient domains along with the option to
> print only UUIDs or names of domains instead of printing the table.
> 
> Option --name prints domain names (one per line) instead of the default
> table. Similarly --uuid prints domain's UUID. The option --table is
> an alias for the default behavior.

I wonder if we also need --id.  --table prints out the id, but then you
have to parse out the first field.  On the other hand, id is only valid
for running guests, so --id + --persistent + --inactive would have no
output unless we make it an error; and we are at least guaranteed that
the id is always parseable (it is names that might have embedded
spacing, and thus difficult to parse out of a table).  At any rate, I'm
less worried about id; getting just uuid or name is a great improvement
in its own right.

> 
> Aditionaly --persistent and/or --transient may be specified to filter

s/Aditionaly/Additionally/

> the output of domains.
> ---
>  tools/virsh.c   |  176 ++++++++++++++++++++++++++++++++++++++++--------------
>  tools/virsh.pod |   20 ++++++-
>  2 files changed, 148 insertions(+), 48 deletions(-)

Thanks for picking up on my suggestions, and implementing it so quickly!

> +
> +    if ((optTable && optName) ||
> +        (optTable && optUUID) ||
> +        (optName && optUUID)) {

I think it might be easier to write this as:

if (optTable + optName + optUUID > 1) {

> +        vshError(ctl, "%s",
> +                 _("Only one argument from --table, --name and --uuid"
> +                   "may be specified."));

which would also make things easier if you later add an --id option.

> @@ -950,77 +986,125 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>          }
>      }
> 
> -    if (desc) {
> -        vshPrintExtra(ctl, "%-5s %-30s %-10s %s\n", _("Id"), _("Name"), _("State"), _("Title"));
> -        vshPrintExtra(ctl, "-----------------------------------------------------------\n");
> -    } else {
> -        vshPrintExtra(ctl, " %-5s %-30s %s\n", _("Id"), _("Name"), _("State"));
> -        vshPrintExtra(ctl, "----------------------------------------------------\n");

The old style printed a variable-length ---- line, depending on whether
title was in the mix...

> +    /* print table header in legacy mode */
> +    if (optTable) {
> +        vshPrintExtra(ctl, " %-5s %-30s %-10s",
> +                      _("Id"), _("Name"), _("State"));
> +        if (optTitle)
> +            vshPrintExtra(ctl, "%-20s", _("Title"));
> +
> +        vshPrintExtra(ctl, "\n"
> +                      "-----------------------------------------------------------\n");

but your new version prints a fixed-width ---- line as if title were
always present.  Not necessarily a show-stopper, but worth thinking about.

> +++ b/tools/virsh.pod
> @@ -280,6 +280,8 @@ The XML also show the NUMA topology information if available.
>  Inject NMI to the guest.
> 
>  =item B<list> [I<--inactive> | I<--all>] [I<--managed-save>] [I<--title>]
> +              [I<--name> | <I--table> | <I--uuid>] [I<--persistent>]

I think we have been using {} to indicate mutually exclusive choices.
Maybe:

{I<--name> | I<--uuid> | [I<--table>]}

as a way to show there are three modes, and --table is the default mode.
 In fact, maybe it's worth a slight change-up to the patch, to expose:

virsh list --output={table|uuid|name}

as a single option with a mandatory argument, rather than having three
separate flag arguments, where if --output is omitted, you default to
--output=table.

But that's all bike-shedding; what you have works, so I don't think it
is worth redoing anything.

> +
> +If I<--name> is specified, domain names are printed instead of the table
> +formated one per line. If I<--uuid> is specified domain's UUID's are printed

s/formated/formatted/

> +instead of names. Flag I<--table> specifies that the legacy table-formated

s/formated/formatted/

> +output should be used. This is the default. All of these are mutually
> +exclusive.
> +
> +Flag I<--persistent> specifies that persistent domains should be printed.
> +Similarly I<--transiet> enables output of transient domains. These flags

s/transiet/transient/

> +may be combined. Default behavior is as though both were specifiedi (Although

s/specifiedi/specified./

> +if any of these flags is specified, the check for the persistence state is done

s/(Although if/(Note that if/

ACK with the virsh.pod typo fixes.

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