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

[John Ferlan]

On 01/11/2016 11:26 AM, Andrea Bolognani wrote:
> 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.
> 

That's the option I took for patches 5, 6, & 7.  Also applied the common
config, live, and current options to other files which also used the
options.

Tks -

John