Re: [libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list
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
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)
+ 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.
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)