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.