[libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list

Peter Krempa pkrempa at redhat.com
Fri Mar 1 22:20:37 UTC 2013


On 03/01/13 19:38, Eric Blake wrote:
> On 03/01/2013 08:43 AM, Peter Krempa wrote:
>> Simplify error handling and mutually exlusive option checking.
>
> s/exlusive/exclusive/
>
>> ---
>>
>> Notes:
>>      I'm thinking of making the VSH_EXCLUSIVE_OPTION macro global
>>      in virsh. Mutually exclusive parameters are checked in many
>>      places throughout virsh and this might really simplify stuff
>>      sometimes.
>
> To make it more generic, you need to tweak it slightly.
>
>
>> +    bool from = vshCommandOptBool(cmd, "from");
>> +    bool parent = vshCommandOptBool(cmd, "parent");
>> +    bool roots = vshCommandOptBool(cmd, "roots");
>> +    bool current = vshCommandOptBool(cmd, "current");
>
> Here, you got lucky that all of the options you are checking can be
> represented as a variable name.  But if we ever have a --two-part option

I actually  helped the luck a bit :)

> that is mutually exclusive, then you need to distinguish between the
> option name and the variable name.
>
>> +#define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2)                               \
>
> That is, I'd define this in terms of
>   VSH_EXCLUSIVE_OPTIONS(COND1, COND2, NAME1, NAME2)

I'd go with two versions. One for the cool ones that actually match and 
the second one as you proposed here.

>
>> +    if (NAME1 && NAME2) {                                                 \
>
> check COND1 and COND2 here
>
>> +        vshError(ctl, _("Options --%s and --%s are mutually exclusive"),  \
>> +                 #NAME1, #NAME2);                                         \
>
> so that NAME1 and NAME2 can include dashes.
>
>> +    VSH_EXCLUSIVE_OPTIONS(tree, name);
>> +    VSH_EXCLUSIVE_OPTIONS(parent, roots);
>> +    VSH_EXCLUSIVE_OPTIONS(parent, tree);
>> +    VSH_EXCLUSIVE_OPTIONS(roots, tree);
>> +    VSH_EXCLUSIVE_OPTIONS(roots, from);
>> +    VSH_EXCLUSIVE_OPTIONS(roots, current);
>> +#undef VSH_EXCLUSIVE_OPTIONS
>
> But this part is slick :)
>
> Feels like a lot of churn in one patch; mixing variable renaming and
> logic flow changes made it a bit harder.  But I'm not sure if it is any
> easier to split into multiple commits now.

I will try to split it, when I'll be doing the more universal exclusive 
option checker.

>
> As written, it seems like this patch works, but I'm worried about
> getting the mutual exclusion check reusable.  Definitely not worth the
> risk to 1.0.3; and maybe someone else wants to chime in on whether we
> need a v2.
>

I will post a v2 ... I just wanted to see if this idea is reasonable.

Peter




More information about the libvir-list mailing list