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