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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
