Re: [PATCH 2/7] virsh-domain: Refactor cmdVcpucount and fix output on inactive domains

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

 



On 04/15/2013 09:11 AM, Peter Krempa wrote:
> This patch factors out the vCPU count retrieval including fallback means into
> vshCPUCountCollect() and removes the duplicated code to retrieve individual
> counts.
> 
> The --current flag (this flag is assumed by default) now works also with
> --maximum or --active without the need to explicitly specify the state of the
> domain that is requested.
> 
> This patch also fixes the output of "virsh vcpucount domain" on inactive
> domains:
> 
> Before:
> $ virsh vcpucount domain
> maximum      config         4
> error: Requested operation is not valid: domain is not running
> current      config         4
> error: Requested operation is not valid: domain is not running
> 
> After:
> $virsh vcpucount domain
> maximum      config         4
> current      config         4

Looks nicer; and I don't think we ever promised that there would always
be exactly four lines of output.  What about the converse case, of
attempting to query the config of a transient domain?  Should you also
clean up that output to avoid error messages?

> ---
>  tools/virsh-domain.c | 258 +++++++++++++++++++++++----------------------------
>  1 file changed, 115 insertions(+), 143 deletions(-)
> 

>  cmdVcpucount(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
> -    bool ret = true;
> +    bool ret = false;
>      bool maximum = vshCommandOptBool(cmd, "maximum");
>      bool active = vshCommandOptBool(cmd, "active");
>      bool config = vshCommandOptBool(cmd, "config");
>      bool live = vshCommandOptBool(cmd, "live");
>      bool current = vshCommandOptBool(cmd, "current");
>      bool all = maximum + active + current + config + live == 0;
> -    int count;
> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> 
> -    /* We want one of each pair of mutually exclusive options; that
> -     * is, use of flags requires exactly two options.  We reject the
> -     * use of more than 2 flags later on.  */
> -    if (maximum + active + current + config + live == 1) {
> -        if (maximum || active) {
> -            vshError(ctl,
> -                     _("when using --%s, one of --config, --live, or --current "
> -                       "must be specified"),
> -                     maximum ? "maximum" : "active");
> -        } else {
> -            vshError(ctl,
> -                     _("when using --%s, either --maximum or --active must be "
> -                       "specified"),
> -                     (current ? "current" : config ? "config" : "live"));
> -        }
> -        return false;
> -    }
> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);

NACK to this line, at least in this location.  It prevents...

> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +    VSH_EXCLUSIVE_OPTIONS_VAR(active, maximum);
> 
>      /* Backwards compatibility: prior to 0.9.4,
>       * VIR_DOMAIN_AFFECT_CURRENT was unsupported, and --current meant

...this backwards-compatibility hack, where '--current --live' must
behave like the modern '--active --live'.

> @@ -5129,140 +5196,45 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd)
>          active = true;
>      }
> 
> -    if (maximum && active) {
> -        vshError(ctl, "%s",
> -                 _("--maximum and --active cannot both be specified"));
> -        return false;
> -    }
> -    if (current + config + live > 1) {
> -        vshError(ctl, "%s",
> -                 _("--config, --live, and --current are mutually exclusive"));
> -        return false;
> -    }

Moving it here would be okay, though.

Everything else seemed okay.

-- 
Eric Blake   eblake redhat com    +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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]