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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list