[libvirt] [PATCH v3 05/14] virsh: Create macro for common "config" option

[Andrea Bolognani]

On Mon, 2016-01-11 at 10:55 -0500, John Ferlan wrote:
>
> > The config option for the 'schedinfo' and 'change-media'
> > commands, while it has a slightly different help text, also
> > serves AFAICT the same purpose and as such should IMHO use the
> > macro you just defined as well.
> 
> BTW: I could do something similar as done for the _FILE w/r/t _helpstr
> for any ".type = VSH_OT_BOOL," options...  To avoid the multiple places
> where I'd pass "N_("affect next boot"), I could change the macro to be:
> 
> 
> #define VIRSH_COMMON_OPT_CONFIG(_helpstr)                 \
>     {.name = "config",                                    \
>      .type = VSH_OT_BOOL,                                 \
>      .help = _helpstr ? _helpstr : N_("affect next boot") \
>     }                                                     \
> 
> and callers be either (NULL) or whatever helpstring - thoughts??

I think that might be pushing it a little too far, because while
browsing the code it would be easy to tell what

  VIRSH_COMMON_OPT_CONFIG(N_("modify/get persistent configuration"))

means but

  VIRSH_COMMON_OPT_CONFIG(NULL)

would be much more obscure.

I'd rather have VIRSH_COMMON_OPT_CONFIG() require its argument and
create extra macros on top of it, like I suggested doing in my
comments on 08/14, eg.

  #define VIRSH_COMMON_OPT_DOMAIN_CONFIG \
      VIRSH_COMMON_OPT_CONFIG(N_("affect next boot"))

because then, while reading the code using it, you would go

  «Oh, right, the usual -config options supported by pretty much
   all commands that act on domains!»

Or at least that's what *I* would go :)

Plus, in some cases, it might be difficult to decide what the
default message should be... See again my comments on 08/14.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team