Re: [libvirt] [PATCH v3 09/14] virsh: Create macros for common "pool" options
On 01/11/2016 10:39 AM, Andrea Bolognani wrote:
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 :)
Well it wasn't my favorite either, but your assumption is right.
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.
Yes... Since I merged all the "domain" options into one commit, I've
done something similar to the "pool" options. Too long to describe, but
it follows the processing used for config, live, and current options.
I've also lift/applied the "helpstr" to each of the # define
VIRSH_COMMON_OPT_*'s in virsh.h (meaning virsh.h is not included in
po/POTFILES.in either). That also picks up a couple more domain options
from virsh-domain.c which use different helpstr's.
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?
OT_DATA and OT_STRING are handled differently... STRING is never
OFLAG_REQ and DATA is always OFLAG_REQ.
So, since it's optional - I created VIRSH_COMMON_OPT_POOL_OPTIONAL, but
it's in patch 2 now.
John
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