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

[Peter Krempa]

On 04/16/13 00:44, Eric Blake wrote:
> 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'.

Hmm, yeah, I didn't port the compatibility code correctly here. Even if 
the check is moved, the compatibility code needs to be updated.

Peter