[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]