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