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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
