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