[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