Re: [libvirt] [PATCH v3 09/14] virsh: Create macros for common "pool" options
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command,
create common macros to be used generically. For virsh-volume, there
are 3 different types of "pool" options - 2 for create, 2 required
for the command, and 10 for string type options. Create 2 new macros
for the create and string type options, but use the virsh.h common
macro for the required for command option.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/virsh-volume.c | 87 ++++++++++++++++------------------------------------
1 file changed, 27 insertions(+), 60 deletions(-)
I really don't like this :)
I see you're trying to get all instances of the 'pool' option to be
handled by macros, and I appreciate that, but in this specific case
I think the resulting code might be a little harder to grasp, which
is something we should avoid unless it brings along huge benefits.
+#define VIRSH_COMMON_OPT_VOLUME_POOL_CREATE \
+ {.name = "pool", \
+ .type = VSH_OT_DATA, \
+ .flags = VSH_OFLAG_REQ, \
+ .help = N_("pool name") \
+ } \
So this is basically the same as VIRSH_COMMON_OPT_POOL, minus "or
uuid" in the help text.
Can we get away with just using VIRSH_COMMON_OPT_POOL, relying on
the fact that you obviously can't specify the UUID of a yet to be
created pool?
Alternatively, since this is used just twice, I'd rather have the
whole definition two times than introduce a potentially confusing
macro. We already have special cases for other options, this can
be a special case as well.
+#define VIRSH_COMMON_OPT_VOLUME_POOL_STRING \
+ {.name = "pool", \
+ .type = VSH_OT_STRING, \
+ .help = N_("pool name or uuid") \
+ } \
Is there any actual difference between VSH_OT_STRING and
VSH_OT_DATA? Can we convert this to VSH_OT_DATA and rename it to
VIRSH_COMMON_OPT_POOL_OPTIONAL or something like that? Or maybe
have *this* as VIRSH_COMMON_OPT_POOL and rename the one
describing a required option to VIRSH_COMMON_OPT_POOL_REQ?
That way you could look at the macro names and immediately know
which one you're supposed to be using, which is IMHO not the
case with the names you're proposing.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team