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