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

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'.