[libvirt] [PATCH v3 00/14] Use macros for more common virsh command options
v2: http://www.redhat.com/archives/libvir-list/2015-December/msg00766.html Changes since v2: Use VIRSH_COMMON_OPT_<optname> for option prefix instead of VIRSH_<optname>_OPT_COMMON Patches have a few thumbs up already, figured I'd post it one last time for perusal and checks of the naming 'algorithm. John Ferlan (14): virsh: Covert VSH_POOL_ macro to VIRSH_COMMON_OPT_ virsh: Move VIRSH_COMMON_OPT_POOL to virsh.h virsh: Create macro for common "domain" option virsh: Create macro for common "persistent" option virsh: Create macro for common "config" option virsh: Create macro for common "live" option virsh: Create macro for common "current" option virsh: Create macro for common "file" option virsh: Create macros for common "pool" options virsh: Create macros for common "vol" options virsh: Have domain-monitor use common "domain" option virsh: have snapshot use common "domain" option virsh: Create macro for common "network" option virsh: Create macro for common "interface" option po/POTFILES.in | 1 + tools/virsh-domain-monitor.c | 77 +--- tools/virsh-domain.c | 911 +++++++++---------------------------------- tools/virsh-interface.c | 37 +- tools/virsh-network.c | 61 +-- tools/virsh-pool.c | 71 ++-- tools/virsh-snapshot.c | 60 +-- tools/virsh-volume.c | 148 ++----- tools/virsh.h | 17 + 9 files changed, 334 insertions(+), 1049 deletions(-) -- 2.5.0
Commit id's 'cf793b00', 'e178688f', 'f9a6110f', '5372d49', and 'e193735' added new VSH_POOL_ macros; however, it was pointed out after push that commit id '834c5720' preferred use of VIRSH_ for the prefix over VSH_. So this patch just changes the VSH_ to VIRSH_ and it changes the naming format from VIRSH_<opt>_OPT_COMMON to VIRSH_COMMON_OPT_<opt>. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 66 +++++++++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index f2f25da..dcac0ba 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -33,42 +33,42 @@ #include "conf/storage_conf.h" #include "virstring.h" -#define VSH_POOL_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL \ {.name = "pool", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ .help = N_("pool name or uuid") \ } \ -#define VSH_POOL_FILE_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL_FILE \ {.name = "file", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ .help = N_("file containing an XML pool description") \ } \ -#define VSH_POOL_BUILD_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL_BUILD \ {.name = "build", \ .type = VSH_OT_BOOL, \ .flags = 0, \ .help = N_("build the pool as normal") \ } \ -#define VSH_POOL_NO_OVERWRITE_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL_NO_OVERWRITE \ {.name = "no-overwrite", \ .type = VSH_OT_BOOL, \ .flags = 0, \ .help = N_("do not overwrite an existing pool of this type") \ } \ -#define VSH_POOL_OVERWRITE_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL_OVERWRITE \ {.name = "overwrite", \ .type = VSH_OT_BOOL, \ .flags = 0, \ .help = N_("overwrite any existing data") \ } \ -#define VSH_POOL_X_AS_OPT_COMMON \ +#define VIRSH_COMMON_OPT_POOL_X_AS \ {.name = "name", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ @@ -188,7 +188,7 @@ static const vshCmdInfo info_pool_autostart[] = { }; static const vshCmdOptDef opts_pool_autostart[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = "disable", .type = VSH_OT_BOOL, @@ -241,10 +241,10 @@ static const vshCmdInfo info_pool_create[] = { }; static const vshCmdOptDef opts_pool_create[] = { - VSH_POOL_FILE_OPT_COMMON, - VSH_POOL_BUILD_OPT_COMMON, - VSH_POOL_NO_OVERWRITE_OPT_COMMON, - VSH_POOL_OVERWRITE_OPT_COMMON, + VIRSH_COMMON_OPT_POOL_FILE, + VIRSH_COMMON_OPT_POOL_BUILD, + VIRSH_COMMON_OPT_POOL_NO_OVERWRITE, + VIRSH_COMMON_OPT_POOL_OVERWRITE, {.name = NULL} }; @@ -297,7 +297,7 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd) } static const vshCmdOptDef opts_pool_define_as[] = { - VSH_POOL_X_AS_OPT_COMMON, + VIRSH_COMMON_OPT_POOL_X_AS, {.name = NULL} }; @@ -413,10 +413,10 @@ static const vshCmdInfo info_pool_create_as[] = { }; static const vshCmdOptDef opts_pool_create_as[] = { - VSH_POOL_X_AS_OPT_COMMON, - VSH_POOL_BUILD_OPT_COMMON, - VSH_POOL_NO_OVERWRITE_OPT_COMMON, - VSH_POOL_OVERWRITE_OPT_COMMON, + VIRSH_COMMON_OPT_POOL_X_AS, + VIRSH_COMMON_OPT_POOL_BUILD, + VIRSH_COMMON_OPT_POOL_NO_OVERWRITE, + VIRSH_COMMON_OPT_POOL_OVERWRITE, {.name = NULL} }; @@ -484,7 +484,7 @@ static const vshCmdInfo info_pool_define[] = { }; static const vshCmdOptDef opts_pool_define[] = { - VSH_POOL_FILE_OPT_COMMON, + VIRSH_COMMON_OPT_POOL_FILE, {.name = NULL} }; @@ -575,9 +575,9 @@ static const vshCmdInfo info_pool_build[] = { }; static const vshCmdOptDef opts_pool_build[] = { - VSH_POOL_OPT_COMMON, - VSH_POOL_NO_OVERWRITE_OPT_COMMON, - VSH_POOL_OVERWRITE_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, + VIRSH_COMMON_OPT_POOL_NO_OVERWRITE, + VIRSH_COMMON_OPT_POOL_OVERWRITE, {.name = NULL} }; @@ -625,7 +625,7 @@ static const vshCmdInfo info_pool_destroy[] = { }; static const vshCmdOptDef opts_pool_destroy[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -665,7 +665,7 @@ static const vshCmdInfo info_pool_delete[] = { }; static const vshCmdOptDef opts_pool_delete[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -705,7 +705,7 @@ static const vshCmdInfo info_pool_refresh[] = { }; static const vshCmdOptDef opts_pool_refresh[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -745,7 +745,7 @@ static const vshCmdInfo info_pool_dumpxml[] = { }; static const vshCmdOptDef opts_pool_dumpxml[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = "inactive", .type = VSH_OT_BOOL, @@ -1596,7 +1596,7 @@ static const vshCmdInfo info_pool_info[] = { }; static const vshCmdOptDef opts_pool_info[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -1673,7 +1673,7 @@ static const vshCmdInfo info_pool_name[] = { }; static const vshCmdOptDef opts_pool_name[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -1705,10 +1705,10 @@ static const vshCmdInfo info_pool_start[] = { }; static const vshCmdOptDef opts_pool_start[] = { - VSH_POOL_OPT_COMMON, - VSH_POOL_BUILD_OPT_COMMON, - VSH_POOL_NO_OVERWRITE_OPT_COMMON, - VSH_POOL_OVERWRITE_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, + VIRSH_COMMON_OPT_POOL_BUILD, + VIRSH_COMMON_OPT_POOL_NO_OVERWRITE, + VIRSH_COMMON_OPT_POOL_OVERWRITE, {.name = NULL} }; @@ -1766,7 +1766,7 @@ static const vshCmdInfo info_pool_undefine[] = { }; static const vshCmdOptDef opts_pool_undefine[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -1806,7 +1806,7 @@ static const vshCmdInfo info_pool_uuid[] = { }; static const vshCmdOptDef opts_pool_uuid[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; @@ -1843,7 +1843,7 @@ static const vshCmdInfo info_pool_edit[] = { }; static const vshCmdOptDef opts_pool_edit[] = { - VSH_POOL_OPT_COMMON, + VIRSH_COMMON_OPT_POOL, {.name = NULL} }; -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Commit id's 'cf793b00', 'e178688f', 'f9a6110f', '5372d49', and 'e193735' added new VSH_POOL_ macros; however, it was pointed out after push that commit id '834c5720' preferred use of VIRSH_ for the prefix over VSH_.
So this patch just changes the VSH_ to VIRSH_ and it changes the naming format from VIRSH_<opt>_OPT_COMMON to VIRSH_COMMON_OPT_<opt>.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 66 +++++++++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 33 deletions(-)
s/Covert/Convert/ in the commit message. ACK with that fixed. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Since it can be used by other virsh*.c modules, move the macro to virsh.h and add virsh.h to POTFILES.in (since there are translatable strings). Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + tools/virsh-pool.c | 7 ------- tools/virsh.h | 10 ++++++++++ 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 82e8d3e..46d3940 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -265,6 +265,7 @@ src/xenconfig/xen_xm.c tests/virpolkittest.c tools/libvirt-guests.sh.in tools/virsh.c +tools/virsh.h tools/virsh-console.c tools/virsh-domain-monitor.c tools/virsh-domain.c diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index dcac0ba..1644a8a 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -33,13 +33,6 @@ #include "conf/storage_conf.h" #include "virstring.h" -#define VIRSH_COMMON_OPT_POOL \ - {.name = "pool", \ - .type = VSH_OT_DATA, \ - .flags = VSH_OFLAG_REQ, \ - .help = N_("pool name or uuid") \ - } \ - #define VIRSH_COMMON_OPT_POOL_FILE \ {.name = "file", \ .type = VSH_OT_DATA, \ diff --git a/tools/virsh.h b/tools/virsh.h index 3402408..d9d5696 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -59,6 +59,16 @@ # define VIRSH_CMD_GRP_HOST_AND_HV "Host and Hypervisor" # define VIRSH_CMD_GRP_VIRSH "Virsh itself" +/* + * Common command options + */ +# define VIRSH_COMMON_OPT_POOL \ + {.name = "pool", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("pool name or uuid") \ + } \ + typedef struct _virshControl virshControl; typedef virshControl *virshControlPtr; -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Since it can be used by other virsh*.c modules, move the macro to virsh.h and add virsh.h to POTFILES.in (since there are translatable strings).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + tools/virsh-pool.c | 7 ------- tools/virsh.h | 10 ++++++++++ 3 files changed, 11 insertions(+), 7 deletions(-)
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "domain",' entries are replaced, just those that have the common .help string of "domain name, id or uuid" and those that are required. Other instances won't take all 3 options, but some subset of those options as directed by the virshCommandOptDomainBy flags argument or in some instances where the domain is not a required option. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 445 +++++++++------------------------------------------ tools/virsh.h | 7 + 2 files changed, 81 insertions(+), 371 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7c65bf4..29b18c9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -63,7 +63,6 @@ # define SA_SIGINFO 0 #endif - static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -206,11 +205,7 @@ static const vshCmdInfo info_attach_device[] = { }; static const vshCmdOptDef opts_attach_device[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -308,11 +303,7 @@ static const vshCmdInfo info_attach_disk[] = { }; static const vshCmdOptDef opts_attach_disk[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "source", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, @@ -808,11 +799,7 @@ static const vshCmdInfo info_attach_interface[] = { }; static const vshCmdOptDef opts_attach_interface[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "type", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1135,11 +1122,7 @@ static const vshCmdInfo info_autostart[] = { }; static const vshCmdOptDef opts_autostart[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "disable", .type = VSH_OT_BOOL, .help = N_("disable autostarting") @@ -1191,11 +1174,7 @@ static const vshCmdInfo info_blkdeviotune[] = { }; static const vshCmdOptDef opts_blkdeviotune[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1534,11 +1513,7 @@ static const vshCmdInfo info_blkiotune[] = { }; static const vshCmdOptDef opts_blkiotune[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "weight", .type = VSH_OT_INT, .help = N_("IO Weight") @@ -2007,11 +1982,7 @@ static const vshCmdInfo info_block_commit[] = { }; static const vshCmdOptDef opts_block_commit[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -2227,11 +2198,7 @@ static const vshCmdInfo info_block_copy[] = { }; static const vshCmdOptDef opts_block_copy[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -2532,11 +2499,7 @@ static const vshCmdInfo info_block_job[] = { }; static const vshCmdOptDef opts_block_job[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -2777,11 +2740,7 @@ static const vshCmdInfo info_block_pull[] = { }; static const vshCmdOptDef opts_block_pull[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -2916,11 +2875,7 @@ static const vshCmdInfo info_block_resize[] = { }; static const vshCmdOptDef opts_block_resize[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -2984,11 +2939,7 @@ static const vshCmdInfo info_console[] = { }; static const vshCmdOptDef opts_console[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "devname", /* sc_prohibit_devname */ .type = VSH_OT_STRING, .help = N_("character device name") @@ -3082,11 +3033,7 @@ static const vshCmdInfo info_domif_setlink[] = { }; static const vshCmdOptDef opts_domif_setlink[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "interface", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3269,11 +3216,7 @@ static const vshCmdInfo info_domiftune[] = { }; static const vshCmdOptDef opts_domiftune[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "interface", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3479,11 +3422,7 @@ static const vshCmdInfo info_suspend[] = { }; static const vshCmdOptDef opts_suspend[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -3525,11 +3464,7 @@ static const vshCmdInfo info_dom_pm_suspend[] = { }; static const vshCmdOptDef opts_dom_pm_suspend[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "target", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3607,11 +3542,7 @@ static const vshCmdInfo info_dom_pm_wakeup[] = { }; static const vshCmdOptDef opts_dom_pm_wakeup[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -4257,11 +4188,7 @@ static const vshCmdInfo info_save[] = { }; static const vshCmdOptDef opts_save[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -4727,11 +4654,7 @@ static const vshCmdInfo info_managedsave[] = { }; static const vshCmdOptDef opts_managedsave[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "bypass-cache", .type = VSH_OT_BOOL, .help = N_("avoid file system cache when saving") @@ -4851,11 +4774,7 @@ static const vshCmdInfo info_managedsaveremove[] = { }; static const vshCmdOptDef opts_managedsaveremove[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -4910,11 +4829,7 @@ static const vshCmdInfo info_schedinfo[] = { }; static const vshCmdOptDef opts_schedinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "weight", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ_OPT, @@ -5238,11 +5153,7 @@ static const vshCmdInfo info_dump[] = { }; static const vshCmdOptDef opts_dump[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -5421,11 +5332,7 @@ static const vshCmdInfo info_screenshot[] = { }; static const vshCmdOptDef opts_screenshot[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_STRING, .help = N_("where to store the screenshot") @@ -5568,11 +5475,7 @@ static const vshCmdInfo info_set_user_password[] = { }; static const vshCmdOptDef opts_set_user_password[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "user", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -5636,11 +5539,7 @@ static const vshCmdInfo info_resume[] = { }; static const vshCmdOptDef opts_resume[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -5679,11 +5578,7 @@ static const vshCmdInfo info_shutdown[] = { }; static const vshCmdOptDef opts_shutdown[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "mode", .type = VSH_OT_STRING, .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") @@ -5768,11 +5663,7 @@ static const vshCmdInfo info_reboot[] = { }; static const vshCmdOptDef opts_reboot[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "mode", .type = VSH_OT_STRING, .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") @@ -5852,11 +5743,7 @@ static const vshCmdInfo info_reset[] = { }; static const vshCmdOptDef opts_reset[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -5895,11 +5782,7 @@ static const vshCmdInfo info_domjobinfo[] = { }; static const vshCmdOptDef opts_domjobinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "completed", .type = VSH_OT_BOOL, .help = N_("return statistics of a recently completed job") @@ -6197,11 +6080,7 @@ static const vshCmdInfo info_domjobabort[] = { }; static const vshCmdOptDef opts_domjobabort[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -6235,11 +6114,7 @@ static const vshCmdInfo info_vcpucount[] = { }; static const vshCmdOptDef opts_vcpucount[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "maximum", .type = VSH_OT_BOOL, .help = N_("get maximum count of vcpus") @@ -6450,11 +6325,7 @@ static const vshCmdInfo info_vcpuinfo[] = { }; static const vshCmdOptDef opts_vcpuinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "pretty", .type = VSH_OT_BOOL, .help = N_("return human readable output") @@ -6565,11 +6436,7 @@ static const vshCmdInfo info_vcpupin[] = { }; static const vshCmdOptDef opts_vcpupin[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "vcpu", .type = VSH_OT_INT, .help = N_("vcpu number") @@ -6777,11 +6644,7 @@ static const vshCmdInfo info_emulatorpin[] = { }; static const vshCmdOptDef opts_emulatorpin[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "cpulist", .type = VSH_OT_STRING, .flags = VSH_OFLAG_EMPTY_OK, @@ -6894,11 +6757,7 @@ static const vshCmdInfo info_setvcpus[] = { }; static const vshCmdOptDef opts_setvcpus[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "count", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -6995,11 +6854,7 @@ static const vshCmdInfo info_iothreadinfo[] = { {.name = NULL} }; static const vshCmdOptDef opts_iothreadinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -7084,11 +6939,7 @@ static const vshCmdInfo info_iothreadpin[] = { }; static const vshCmdOptDef opts_iothreadpin[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "iothread", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -7179,11 +7030,7 @@ static const vshCmdInfo info_iothreadadd[] = { }; static const vshCmdOptDef opts_iothreadadd[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "id", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -7257,11 +7104,7 @@ static const vshCmdInfo info_iothreaddel[] = { }; static const vshCmdOptDef opts_iothreaddel[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "id", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -7554,11 +7397,7 @@ static const vshCmdInfo info_cpu_stats[] = { }; static const vshCmdOptDef opts_cpu_stats[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "total", .type = VSH_OT_BOOL, .help = N_("Show total statistics only") @@ -7905,11 +7744,7 @@ static const vshCmdInfo info_destroy[] = { }; static const vshCmdOptDef opts_destroy[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "graceful", .type = VSH_OT_BOOL, .help = N_("terminate gracefully") @@ -7962,11 +7797,7 @@ static const vshCmdInfo info_desc[] = { }; static const vshCmdOptDef opts_desc[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "live", .type = VSH_OT_BOOL, .help = N_("modify/get running state") @@ -8141,11 +7972,7 @@ static const vshCmdInfo info_metadata[] = { }; static const vshCmdOptDef opts_metadata[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "uri", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -8300,11 +8127,7 @@ static const vshCmdInfo info_inject_nmi[] = { }; static const vshCmdOptDef opts_inject_nmi[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -8338,11 +8161,7 @@ static const vshCmdInfo info_send_key[] = { }; static const vshCmdOptDef opts_send_key[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "codeset", .type = VSH_OT_STRING, .flags = VSH_OFLAG_REQ_OPT, @@ -8438,11 +8257,7 @@ static const vshCmdInfo info_send_process_signal[] = { }; static const vshCmdOptDef opts_send_process_signal[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "pid", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -8547,11 +8362,7 @@ static const vshCmdInfo info_setmem[] = { }; static const vshCmdOptDef opts_setmem[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "kilobytes", .type = VSH_OT_ALIAS, .help = "size" @@ -8641,11 +8452,7 @@ static const vshCmdInfo info_setmaxmem[] = { }; static const vshCmdOptDef opts_setmaxmem[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "kilobytes", .type = VSH_OT_ALIAS, .help = "size" @@ -8742,11 +8549,7 @@ static const vshCmdInfo info_memtune[] = { }; static const vshCmdOptDef opts_memtune[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "hard-limit", .type = VSH_OT_INT, .help = N_("Max memory, as scaled integer (default KiB)") @@ -8932,11 +8735,7 @@ static const vshCmdInfo info_numatune[] = { }; static const vshCmdOptDef opts_numatune[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "mode", .type = VSH_OT_STRING, .help = N_("NUMA mode, one of strict, preferred and interleave \n" @@ -9080,11 +8879,7 @@ static const vshCmdInfo info_qemu_monitor_command[] = { }; static const vshCmdOptDef opts_qemu_monitor_command[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "hmp", .type = VSH_OT_BOOL, .help = N_("command is in human monitor protocol") @@ -9374,11 +9169,7 @@ static const vshCmdInfo info_qemu_agent_command[] = { }; static const vshCmdOptDef opts_qemu_agent_command[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "timeout", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ_OPT, @@ -9501,11 +9292,7 @@ static const vshCmdInfo info_lxc_enter_namespace[] = { }; static const vshCmdOptDef opts_lxc_enter_namespace[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "noseclabel", .type = VSH_OT_BOOL, .help = N_("Do not change process security label") @@ -9644,11 +9431,7 @@ static const vshCmdInfo info_dumpxml[] = { }; static const vshCmdOptDef opts_dumpxml[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("show inactive defined XML") @@ -9868,11 +9651,7 @@ static const vshCmdInfo info_domrename[] = { }; static const vshCmdOptDef opts_domrename[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "new-name", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -10001,11 +9780,7 @@ static const vshCmdInfo info_migrate[] = { }; static const vshCmdOptDef opts_migrate[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "desturi", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -10371,11 +10146,7 @@ static const vshCmdInfo info_migrate_setmaxdowntime[] = { }; static const vshCmdOptDef opts_migrate_setmaxdowntime[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "downtime", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -10426,11 +10197,7 @@ static const vshCmdInfo info_migrate_compcache[] = { }; static const vshCmdOptDef opts_migrate_compcache[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "size", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ_OPT, @@ -10487,11 +10254,7 @@ static const vshCmdInfo info_migrate_setspeed[] = { }; static const vshCmdOptDef opts_migrate_setspeed[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "bandwidth", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, @@ -10537,11 +10300,7 @@ static const vshCmdInfo info_migrate_getspeed[] = { }; static const vshCmdOptDef opts_migrate_getspeed[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -10582,11 +10341,7 @@ static const vshCmdInfo info_domdisplay[] = { }; static const vshCmdOptDef opts_domdisplay[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "include-password", .type = VSH_OT_BOOL, .help = N_("includes the password into the connection URI if available") @@ -10805,11 +10560,7 @@ static const vshCmdInfo info_vncdisplay[] = { }; static const vshCmdOptDef opts_vncdisplay[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -10890,11 +10641,7 @@ static const vshCmdInfo info_ttyconsole[] = { }; static const vshCmdOptDef opts_ttyconsole[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -10950,11 +10697,7 @@ static const vshCmdInfo info_domhostname[] = { }; static const vshCmdOptDef opts_domhostname[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -11113,11 +10856,7 @@ static const vshCmdInfo info_detach_device[] = { }; static const vshCmdOptDef opts_detach_device[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -11214,11 +10953,7 @@ static const vshCmdInfo info_update_device[] = { }; static const vshCmdOptDef opts_update_device[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -11316,11 +11051,7 @@ static const vshCmdInfo info_detach_interface[] = { }; static const vshCmdOptDef opts_detach_interface[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "type", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -11736,11 +11467,7 @@ static const vshCmdInfo info_detach_disk[] = { }; static const vshCmdOptDef opts_detach_disk[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "target", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -11852,11 +11579,7 @@ static const vshCmdInfo info_edit[] = { }; static const vshCmdOptDef opts_edit[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "skip-validate", .type = VSH_OT_BOOL, .help = N_("skip validation of the XML against the schema") @@ -12705,11 +12428,7 @@ static const vshCmdInfo info_change_media[] = { }; static const vshCmdOptDef opts_change_media[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -12877,11 +12596,7 @@ static const vshCmdInfo info_domfstrim[] = { }; static const vshCmdOptDef opts_domfstrim[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "minimum", .type = VSH_OT_INT, .help = N_("Just a hint to ignore contiguous " @@ -12934,11 +12649,7 @@ static const vshCmdInfo info_domfsfreeze[] = { }; static const vshCmdOptDef opts_domfsfreeze[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "mountpoint", .type = VSH_OT_ARGV, .help = N_("mountpoint path to be frozen") @@ -12991,11 +12702,7 @@ static const vshCmdInfo info_domfsthaw[] = { }; static const vshCmdOptDef opts_domfsthaw[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "mountpoint", .type = VSH_OT_ARGV, .help = N_("mountpoint path to be thawed") @@ -13048,11 +12755,7 @@ static const vshCmdInfo info_domfsinfo[] = { }; static const vshCmdOptDef opts_domfsinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; diff --git a/tools/virsh.h b/tools/virsh.h index d9d5696..2101a00 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -69,6 +69,13 @@ .help = N_("pool name or uuid") \ } \ +# define VIRSH_COMMON_OPT_DOMAIN \ + {.name = "domain", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("domain name, id or uuid") \ + } \ + typedef struct _virshControl virshControl; typedef virshControl *virshControlPtr; -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "domain",' entries are replaced, just those that have the common .help string of "domain name, id or uuid" and those that are required. Other instances won't take all 3 options, but some subset of those options as directed by the virshCommandOptDomainBy flags argument or in some instances where the domain is not a required option. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 445 +++++++++------------------------------------------ tools/virsh.h | 7 + 2 files changed, 81 insertions(+), 371 deletions(-)
The domain option for the 'undefine' command takes all three forms[1] even though the help string says otherwise. I think the code is okay as it is and VIRSH_COMMON_OPT_DOMAIN should be used there as well: if you agree, please squash in the appropriate change before pushing. If you think the code should be fixed instead, do so in a separate patch :) ACK with the above taken into account. Cheers. [1] It's processed using virshCommandOptDomain() -- Andrea Bolognani Software Engineer - Virtualization Team
On 01/11/2016 05:57 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 a common macro to be used generically. Note that not all '{.name = "domain",' entries are replaced, just those that have the common .help string of "domain name, id or uuid" and those that are required.
Other instances won't take all 3 options, but some subset of those options as directed by the virshCommandOptDomainBy flags argument or in some instances where the domain is not a required option.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 445 +++++++++------------------------------------------ tools/virsh.h | 7 + 2 files changed, 81 insertions(+), 371 deletions(-)
The domain option for the 'undefine' command takes all three forms[1] even though the help string says otherwise.
I think the code is okay as it is and VIRSH_COMMON_OPT_DOMAIN should be used there as well: if you agree, please squash in the appropriate change before pushing. If you think the code should be fixed instead, do so in a separate patch :)
I agree - the undefine seems to be able to use the common name. When searching - I went on common helpstr value. Tks - John
ACK with the above taken into account.
Cheers.
[1] It's processed using virshCommandOptDomain() -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "persistent",' entries are replaced, just those that have the common .help string of "make live change persistent". Non replaced instances are unique to the command. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 29b18c9..476ba58 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -63,6 +63,12 @@ # define SA_SIGINFO 0 #endif +#define VIRSH_COMMON_OPT_DOMAIN_PERSISTENT \ + {.name = "persistent", \ + .type = VSH_OT_BOOL, \ + .help = N_("make live change persistent") \ + } \ + static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -211,10 +217,7 @@ static const vshCmdOptDef opts_attach_device[] = { .flags = VSH_OFLAG_REQ, .help = N_("XML file") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -374,10 +377,7 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_BOOL, .help = N_("print XML document rather than attach the disk") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -834,10 +834,7 @@ static const vshCmdOptDef opts_attach_interface[] = { .type = VSH_OT_STRING, .help = N_("control domain's outgoing traffics") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -10862,10 +10859,7 @@ static const vshCmdOptDef opts_detach_device[] = { .flags = VSH_OFLAG_REQ, .help = N_("XML file") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -10959,10 +10953,7 @@ static const vshCmdOptDef opts_update_device[] = { .flags = VSH_OFLAG_REQ, .help = N_("XML file") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -11061,10 +11052,7 @@ static const vshCmdOptDef opts_detach_interface[] = { .type = VSH_OT_STRING, .help = N_("MAC address") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") @@ -11473,10 +11461,7 @@ static const vshCmdOptDef opts_detach_disk[] = { .flags = VSH_OFLAG_REQ, .help = N_("target of disk device") }, - {.name = "persistent", - .type = VSH_OT_BOOL, - .help = N_("make live change persistent") - }, + VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, {.name = "config", .type = VSH_OT_BOOL, .help = N_("affect next boot") -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "persistent",' entries are replaced, just those that have the common .help string of "make live change persistent".
Non replaced instances are unique to the command.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-)
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "config",' entries are replaced, just those that have the common .help string of "affect next boot". Non replaced instances are unique to the command. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 116 +++++++++++++-------------------------------------- 1 file changed, 28 insertions(+), 88 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 476ba58..ba6ba7f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -69,6 +69,12 @@ .help = N_("make live change persistent") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_CONFIG \ + {.name = "config", \ + .type = VSH_OT_BOOL, \ + .help = N_("affect next boot") \ + } \ + static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -218,10 +224,7 @@ static const vshCmdOptDef opts_attach_device[] = { .help = N_("XML file") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -378,10 +381,7 @@ static const vshCmdOptDef opts_attach_disk[] = { .help = N_("print XML document rather than attach the disk") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -835,10 +835,7 @@ static const vshCmdOptDef opts_attach_interface[] = { .help = N_("control domain's outgoing traffics") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -1281,10 +1278,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .type = VSH_OT_INT, .help = N_("I/O size in bytes") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -1535,10 +1529,7 @@ static const vshCmdOptDef opts_blkiotune[] = { .type = VSH_OT_STRING, .help = N_("per-device bytes wrote per second, in the form of /path/to/device,write_bytes_sec,...") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -3045,10 +3036,7 @@ static const vshCmdOptDef opts_domif_setlink[] = { .type = VSH_OT_ALIAS, .help = "config" }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = NULL} }; @@ -3227,10 +3215,7 @@ static const vshCmdOptDef opts_domiftune[] = { .type = VSH_OT_STRING, .help = N_("control domain's outgoing traffics") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -6443,10 +6428,7 @@ static const vshCmdOptDef opts_vcpupin[] = { .flags = VSH_OFLAG_EMPTY_OK, .help = N_("host cpu number(s) to set, or omit option to query") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -6647,10 +6629,7 @@ static const vshCmdOptDef opts_emulatorpin[] = { .flags = VSH_OFLAG_EMPTY_OK, .help = N_("host cpu number(s) to set, or omit option to query") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -6764,10 +6743,7 @@ static const vshCmdOptDef opts_setvcpus[] = { .type = VSH_OT_BOOL, .help = N_("set maximum limit on next boot") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -6852,10 +6828,7 @@ static const vshCmdInfo info_iothreadinfo[] = { }; static const vshCmdOptDef opts_iothreadinfo[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -6947,10 +6920,7 @@ static const vshCmdOptDef opts_iothreadpin[] = { .flags = VSH_OFLAG_REQ, .help = N_("host cpu number(s) to set") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -7033,10 +7003,7 @@ static const vshCmdOptDef opts_iothreadadd[] = { .flags = VSH_OFLAG_REQ, .help = N_("iothread for the new IOThread") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -7107,10 +7074,7 @@ static const vshCmdOptDef opts_iothreaddel[] = { .flags = VSH_OFLAG_REQ, .help = N_("iothread_id for the IOThread to delete") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -8369,10 +8333,7 @@ static const vshCmdOptDef opts_setmem[] = { .flags = VSH_OFLAG_REQ, .help = N_("new memory size, as scaled integer (default KiB)") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -8459,10 +8420,7 @@ static const vshCmdOptDef opts_setmaxmem[] = { .flags = VSH_OFLAG_REQ, .help = N_("new maximum memory size, as scaled integer (default KiB)") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -8563,10 +8521,7 @@ static const vshCmdOptDef opts_memtune[] = { .type = VSH_OT_INT, .help = N_("Min guaranteed memory, as scaled integer (default KiB)") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -8742,10 +8697,7 @@ static const vshCmdOptDef opts_numatune[] = { .type = VSH_OT_STRING, .help = N_("NUMA node selections to set") }, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -10860,10 +10812,7 @@ static const vshCmdOptDef opts_detach_device[] = { .help = N_("XML file") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -10954,10 +10903,7 @@ static const vshCmdOptDef opts_update_device[] = { .help = N_("XML file") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -11053,10 +10999,7 @@ static const vshCmdOptDef opts_detach_interface[] = { .help = N_("MAC address") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") @@ -11462,10 +11405,7 @@ static const vshCmdOptDef opts_detach_disk[] = { .help = N_("target of disk device") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, - {.name = "config", - .type = VSH_OT_BOOL, - .help = N_("affect next boot") - }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, {.name = "live", .type = VSH_OT_BOOL, .help = N_("affect running domain") -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "config",' entries are replaced, just those that have the common .help string of "affect next boot".
Non replaced instances are unique to the command.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 116 +++++++++++++-------------------------------------- 1 file changed, 28 insertions(+), 88 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 476ba58..ba6ba7f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -69,6 +69,12 @@ .help = N_("make live change persistent") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_CONFIG \ + {.name = "config", \ + .type = VSH_OT_BOOL, \ + .help = N_("affect next boot") \ + } \ +
.type and .help are not aligned properly. You also seem to have missed the 'dommemstat' command; however, since that command is defined in virsh-domain-monitor.c you'll have to move the definition of VIRSH_COMMON_OPT_DOMAIN_CONFIG to virsh.h to make it available there. 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. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Mon, 2016-01-11 at 13:21 +0100, 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 a common macro to be used generically. Note that not all '{.name = "config",' entries are replaced, just those that have the common .help string of "affect next boot". Non replaced instances are unique to the command. 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.
To expand, I realize that doing so would go against the first paragraph in your commit message; on the other hand, I believe *not* doing so would go against the second one, as the slightly different help string was probably a mistake to begin with when the options do the exact same thing :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Mon, 2016-01-11 at 13:21 +0100, Andrea Bolognani 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.
Ignore the bit about 'schedinfo': in that case the different help text is warranted and VIRSH_COMMON_OPT_DOMAIN_CONFIG should not be used. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 01/11/2016 07:21 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 a common macro to be used generically. Note that not all '{.name = "config",' entries are replaced, just those that have the common .help string of "affect next boot".
Non replaced instances are unique to the command.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 116 +++++++++++++-------------------------------------- 1 file changed, 28 insertions(+), 88 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 476ba58..ba6ba7f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -69,6 +69,12 @@ .help = N_("make live change persistent") \ } \
+#define VIRSH_COMMON_OPT_DOMAIN_CONFIG \ + {.name = "config", \ + .type = VSH_OT_BOOL, \ + .help = N_("affect next boot") \ + } \ +
.type and .help are not aligned properly.
Good catch.
You also seem to have missed the 'dommemstat' command; however, since that command is defined in virsh-domain-monitor.c you'll have to move the definition of VIRSH_COMMON_OPT_DOMAIN_CONFIG to virsh.h to make it available there.
OK - my search was done more on the count of repetitive options rather than looking for repetitive options in other files, but I can make this one more generic too.
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?? John
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
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
On Mon, Jan 11, 2016 at 05:26:55PM +0100, 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.
Cheers.
Just a question, I know it's to late to change those patches, it's pushed now, but why don't we unify the help string for all the commands? It does the same thing for all commands, there is no reason to have different help string for some commands. And I don't think, that it would break anything. Pavel
On Tue, 2016-01-12 at 10:41 +0100, Pavel Hrdina wrote:
Just a question, I know it's to late to change those patches, it's pushed now, but why don't we unify the help string for all the commands? It does the same thing for all commands, there is no reason to have different help string for some commands. And I don't think, that it would break anything.
The most commonly used help text is "affect next boot", while eg. the help text for the 'schedinfo' command is "get/set value to be used on next boot". In this case it makes sense to have a different help text, because the information can not only be set but also retrieved. That said, if you can come up with a help text that can accurately describe all situations where the 'config' option is used, I would certainly not oppose it :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Tue, Jan 12, 2016 at 10:53:03AM +0100, Andrea Bolognani wrote:
On Tue, 2016-01-12 at 10:41 +0100, Pavel Hrdina wrote:
Just a question, I know it's to late to change those patches, it's pushed now, but why don't we unify the help string for all the commands? It does the same thing for all commands, there is no reason to have different help string for some commands. And I don't think, that it would break anything.
The most commonly used help text is "affect next boot", while eg. the help text for the 'schedinfo' command is "get/set value to be used on next boot".
In this case it makes sense to have a different help text, because the information can not only be set but also retrieved.
Yes, that's true and the "affect next boot" is confusing in this case.
That said, if you can come up with a help text that can accurately describe all situations where the 'config' option is used, I would certainly not oppose it :)
What about "affect offline definition", for live "affect running definition" and for current "affect current definition"? What each command does is under "DESCRIPTION" and there is no need to repeat that information for each option.
Cheers.
-- Andrea Bolognani Software Engineer - Virtualization Team
On 01/12/2016 06:40 AM, Pavel Hrdina wrote:
On Tue, Jan 12, 2016 at 10:53:03AM +0100, Andrea Bolognani wrote:
On Tue, 2016-01-12 at 10:41 +0100, Pavel Hrdina wrote:
Just a question, I know it's to late to change those patches, it's pushed now, but why don't we unify the help string for all the commands? It does the same thing for all commands, there is no reason to have different help string for some commands. And I don't think, that it would break anything.
The most commonly used help text is "affect next boot", while eg. the help text for the 'schedinfo' command is "get/set value to be used on next boot".
In this case it makes sense to have a different help text, because the information can not only be set but also retrieved.
Yes, that's true and the "affect next boot" is confusing in this case.
That said, if you can come up with a help text that can accurately describe all situations where the 'config' option is used, I would certainly not oppose it :)
What about "affect offline definition", for live "affect running definition" and for current "affect current definition"? What each command does is under "DESCRIPTION" and there is no need to repeat that information for each option.
So now that there's only one place to change, adjusting the message is probably going to be a bit easier... Changing the text of the message seemed "out of context" for what I was trying to accomplish though. I don't disagree some messages could use an update especially considering how the code as evolved over time. Going through and making each of the help strings meaningful may be an interesting exercise... My favorite helpstr's found during this exercise were 'N_("file")' and 'N_("XML file")' - those were so (ahem) helpful. With the current code in place a "grep N_\(\" tools/virsh-*.c | sort -i | uniq -c | sort -n" will give you an interesting snapshot of helpstrings. Adding an additional "| grep VIRSH_COMMON_OPT" will drill into the ones that were duplicated numerous times, but now affect multiple commands. John
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "live",' entries are replaced, just those that have the common .help string of "affect running domain". Non replaced instances are unique to the command. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 111 +++++++++++++-------------------------------------- 1 file changed, 27 insertions(+), 84 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ba6ba7f..50d8225 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -75,6 +75,12 @@ .help = N_("affect next boot") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_LIVE \ + {.name = "live", \ + .type = VSH_OT_BOOL, \ + .help = N_("affect running domain") \ + } \ + static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -225,10 +231,7 @@ static const vshCmdOptDef opts_attach_device[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -382,10 +385,7 @@ static const vshCmdOptDef opts_attach_disk[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -836,10 +836,7 @@ static const vshCmdOptDef opts_attach_interface[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -1279,10 +1276,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .help = N_("I/O size in bytes") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -1530,10 +1524,7 @@ static const vshCmdOptDef opts_blkiotune[] = { .help = N_("per-device bytes wrote per second, in the form of /path/to/device,write_bytes_sec,...") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -3216,10 +3207,7 @@ static const vshCmdOptDef opts_domiftune[] = { .help = N_("control domain's outgoing traffics") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -6429,10 +6417,7 @@ static const vshCmdOptDef opts_vcpupin[] = { .help = N_("host cpu number(s) to set, or omit option to query") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -6630,10 +6615,7 @@ static const vshCmdOptDef opts_emulatorpin[] = { .help = N_("host cpu number(s) to set, or omit option to query") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -6744,10 +6726,7 @@ static const vshCmdOptDef opts_setvcpus[] = { .help = N_("set maximum limit on next boot") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -6829,10 +6808,7 @@ static const vshCmdInfo info_iothreadinfo[] = { static const vshCmdOptDef opts_iothreadinfo[] = { VIRSH_COMMON_OPT_DOMAIN, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -6921,10 +6897,7 @@ static const vshCmdOptDef opts_iothreadpin[] = { .help = N_("host cpu number(s) to set") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -7004,10 +6977,7 @@ static const vshCmdOptDef opts_iothreadadd[] = { .help = N_("iothread for the new IOThread") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -7075,10 +7045,7 @@ static const vshCmdOptDef opts_iothreaddel[] = { .help = N_("iothread_id for the IOThread to delete") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -8334,10 +8301,7 @@ static const vshCmdOptDef opts_setmem[] = { .help = N_("new memory size, as scaled integer (default KiB)") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -8421,10 +8385,7 @@ static const vshCmdOptDef opts_setmaxmem[] = { .help = N_("new maximum memory size, as scaled integer (default KiB)") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -8522,10 +8483,7 @@ static const vshCmdOptDef opts_memtune[] = { .help = N_("Min guaranteed memory, as scaled integer (default KiB)") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -8698,10 +8656,7 @@ static const vshCmdOptDef opts_numatune[] = { .help = N_("NUMA node selections to set") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -10813,10 +10768,7 @@ static const vshCmdOptDef opts_detach_device[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -10904,10 +10856,7 @@ static const vshCmdOptDef opts_update_device[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -11000,10 +10949,7 @@ static const vshCmdOptDef opts_detach_interface[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") @@ -11406,10 +11352,7 @@ static const vshCmdOptDef opts_detach_disk[] = { }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, - {.name = "live", - .type = VSH_OT_BOOL, - .help = N_("affect running domain") - }, + VIRSH_COMMON_OPT_DOMAIN_LIVE, {.name = "current", .type = VSH_OT_BOOL, .help = N_("affect current domain") -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "live",' entries are replaced, just those that have the common .help string of "affect running domain".
Non replaced instances are unique to the command.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 111 +++++++++++++-------------------------------------- 1 file changed, 27 insertions(+), 84 deletions(-)
Same comments as for the 'config' option with regard to the 'dommemstat' and 'change-media' commands. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "current",' entries are replaced, just those that have the common .help string of "affect current domain". Non replaced instances are unique to the command. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 111 +++++++++++++-------------------------------------- 1 file changed, 27 insertions(+), 84 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 50d8225..e3ed216 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -81,6 +81,12 @@ .help = N_("affect running domain") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_CURRENT \ + {.name = "current", \ + .type = VSH_OT_BOOL, \ + .help = N_("affect current domain") \ + } \ + static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -232,10 +238,7 @@ static const vshCmdOptDef opts_attach_device[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -386,10 +389,7 @@ static const vshCmdOptDef opts_attach_disk[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -837,10 +837,7 @@ static const vshCmdOptDef opts_attach_interface[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = "print-xml", .type = VSH_OT_BOOL, .help = N_("print XML document rather than attach the interface") @@ -1277,10 +1274,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -1525,10 +1519,7 @@ static const vshCmdOptDef opts_blkiotune[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -3208,10 +3199,7 @@ static const vshCmdOptDef opts_domiftune[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -6418,10 +6406,7 @@ static const vshCmdOptDef opts_vcpupin[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -6616,10 +6601,7 @@ static const vshCmdOptDef opts_emulatorpin[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -6727,10 +6709,7 @@ static const vshCmdOptDef opts_setvcpus[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = "guest", .type = VSH_OT_BOOL, .help = N_("modify cpu state in the guest") @@ -6809,10 +6788,7 @@ static const vshCmdOptDef opts_iothreadinfo[] = { VIRSH_COMMON_OPT_DOMAIN, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -6898,10 +6874,7 @@ static const vshCmdOptDef opts_iothreadpin[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -6978,10 +6951,7 @@ static const vshCmdOptDef opts_iothreadadd[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -7046,10 +7016,7 @@ static const vshCmdOptDef opts_iothreaddel[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -8302,10 +8269,7 @@ static const vshCmdOptDef opts_setmem[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -8386,10 +8350,7 @@ static const vshCmdOptDef opts_setmaxmem[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -8484,10 +8445,7 @@ static const vshCmdOptDef opts_memtune[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -8657,10 +8615,7 @@ static const vshCmdOptDef opts_numatune[] = { }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -10769,10 +10724,7 @@ static const vshCmdOptDef opts_detach_device[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -10857,10 +10809,7 @@ static const vshCmdOptDef opts_update_device[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = "force", .type = VSH_OT_BOOL, .help = N_("force device update") @@ -10950,10 +10899,7 @@ static const vshCmdOptDef opts_detach_interface[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; @@ -11353,10 +11299,7 @@ static const vshCmdOptDef opts_detach_disk[] = { VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, - {.name = "current", - .type = VSH_OT_BOOL, - .help = N_("affect current domain") - }, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, {.name = NULL} }; -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "current",' entries are replaced, just those that have the common .help string of "affect current domain".
Non replaced instances are unique to the command.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 111 +++++++++++++-------------------------------------- 1 file changed, 27 insertions(+), 84 deletions(-)
Same comments as for the 'config' option with regard to the 'dommemstat' command. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "file",' entries are replaced, just those that use VSH_OT_DATA and VSH_OFLAG_REQ. Replacement of this option is a bit trickier, since the .helpstr changes from command to command. Also because if the N_() I18N for each, it's also not possible to just copy the string. So, replace the enter right side of the .helpstr = with the _helpstr macro argument. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 85 +++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 65 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e3ed216..ce698c3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -87,6 +87,13 @@ .help = N_("affect current domain") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_FILE(_helpstr) \ + {.name = "file", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = _helpstr \ + } \ + static virDomainPtr virshLookupDomainInternal(vshControl *ctl, const char *cmdname, @@ -230,11 +237,7 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("XML file") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("XML file")), VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, @@ -4147,11 +4150,7 @@ static const vshCmdInfo info_save[] = { static const vshCmdOptDef opts_save[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("where to save the data") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("where to save the data")), {.name = "bypass-cache", .type = VSH_OT_BOOL, .help = N_("avoid file system cache when saving") @@ -4408,11 +4407,7 @@ static const vshCmdInfo info_save_image_dumpxml[] = { }; static const vshCmdOptDef opts_save_image_dumpxml[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("saved state file to read") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("saved state file to read")), {.name = "security-info", .type = VSH_OT_BOOL, .help = N_("include security sensitive information in XML dump") @@ -4461,11 +4456,7 @@ static const vshCmdInfo info_save_image_define[] = { }; static const vshCmdOptDef opts_save_image_define[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("saved state file to modify") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("saved state file to modify")), {.name = "xml", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -4533,11 +4524,7 @@ static const vshCmdInfo info_save_image_edit[] = { }; static const vshCmdOptDef opts_save_image_edit[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("saved state file to edit") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("saved state file to edit")), {.name = "running", .type = VSH_OT_BOOL, .help = N_("set domain to be running on restore") @@ -5031,11 +5018,7 @@ static const vshCmdInfo info_restore[] = { }; static const vshCmdOptDef opts_restore[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("the state to restore") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("the state to restore")), {.name = "bypass-cache", .type = VSH_OT_BOOL, .help = N_("avoid file system cache when restoring") @@ -5112,11 +5095,7 @@ static const vshCmdInfo info_dump[] = { static const vshCmdOptDef opts_dump[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("where to dump the core") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("where to dump the core")), {.name = "live", .type = VSH_OT_BOOL, .help = N_("perform a live core dump if supported") @@ -7073,11 +7052,7 @@ static const vshCmdInfo info_cpu_compare[] = { }; static const vshCmdOptDef opts_cpu_compare[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("file containing an XML CPU description") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("file containing an XML CPU description")), {.name = "error", .type = VSH_OT_BOOL, .help = N_("report error if CPUs are incompatible") @@ -7175,11 +7150,7 @@ static const vshCmdInfo info_cpu_baseline[] = { }; static const vshCmdOptDef opts_cpu_baseline[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("file containing XML CPU descriptions") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("file containing XML CPU descriptions")), {.name = "features", .type = VSH_OT_BOOL, .help = N_("Show features that are part of the CPU model type") @@ -7477,11 +7448,7 @@ static const vshCmdInfo info_create[] = { }; static const vshCmdOptDef opts_create[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("file containing an XML domain description") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("file containing an XML domain description")), #ifndef WIN32 {.name = "console", .type = VSH_OT_BOOL, @@ -7577,11 +7544,7 @@ static const vshCmdInfo info_define[] = { }; static const vshCmdOptDef opts_define[] = { - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("file containing an XML domain description") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("file containing an XML domain description")), {.name = "validate", .type = VSH_OT_BOOL, .help = N_("validate the XML against the schema") @@ -10716,11 +10679,7 @@ static const vshCmdInfo info_detach_device[] = { static const vshCmdOptDef opts_detach_device[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("XML file") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("XML file")), VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, @@ -10801,11 +10760,7 @@ static const vshCmdInfo info_update_device[] = { static const vshCmdOptDef opts_update_device[] = { VIRSH_COMMON_OPT_DOMAIN, - {.name = "file", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("XML file") - }, + VIRSH_COMMON_OPT_DOMAIN_FILE(N_("XML file")), VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "file",' entries are replaced, just those that use VSH_OT_DATA and VSH_OFLAG_REQ. Replacement of this option is a bit trickier, since the .helpstr changes from command to command. Also because if the N_() I18N for each, it's also not possible to just copy the string. So, replace the enter right side of the .helpstr = with the _helpstr ----- -------- - ^ entire ^ .help ^ assignment (?) macro argument. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 85 +++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 65 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e3ed216..ce698c3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -87,6 +87,13 @@ .help = N_("affect current domain") \ } \ +#define VIRSH_COMMON_OPT_DOMAIN_FILE(_helpstr) \ + {.name = "file", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = _helpstr \ + } \ +
I don't think this should be specific to domain commands. I'd rather rename it to VIRSH_COMMON_OPT_FILE(), move it to virsh.h and use it for iface-define net-create net-define nodedev-create nwfilter-define secret-define vol-create vol-create-from vol-upload vol-download too. Even better, you could change #define VIRSH_COMMON_OPT_POOL_FILE \ VIRSH_COMMON_OPT_FILE(N_("file containing an XML pool description")) and add #define VIRSH_COMMON_OPT_VOLUME_FILE \ VIRSH_COMMON_OPT_FILE(N_("file containing an XML vol description")) to use in the vol-* commands listed above. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 01/11/2016 09:38 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 a common macro to be used generically. Note that not all '{.name = "file",' entries are replaced, just those that use VSH_OT_DATA and VSH_OFLAG_REQ.
Replacement of this option is a bit trickier, since the .helpstr changes from command to command. Also because if the N_() I18N for each, it's also not possible to just copy the string. So, replace the enter right side of the .helpstr = with the _helpstr ----- -------- - ^ entire ^ .help ^ assignment (?)
I've nixed the whole paragraph
macro argument.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 85 +++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 65 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e3ed216..ce698c3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -87,6 +87,13 @@ .help = N_("affect current domain") \ } \
+#define VIRSH_COMMON_OPT_DOMAIN_FILE(_helpstr) \ + {.name = "file", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = _helpstr \ + } \ +
I don't think this should be specific to domain commands. I'd rather rename it to VIRSH_COMMON_OPT_FILE(), move it to virsh.h and use it for
iface-define net-create net-define nodedev-create nwfilter-define secret-define vol-create vol-create-from vol-upload vol-download
too. Even better, you could change
#define VIRSH_COMMON_OPT_POOL_FILE \ VIRSH_COMMON_OPT_FILE(N_("file containing an XML pool description"))
and add
#define VIRSH_COMMON_OPT_VOLUME_FILE \ VIRSH_COMMON_OPT_FILE(N_("file containing an XML vol description"))
to use in the vol-* commands listed above.
All are adjusted now to use a common virsh.h VIRSH_COMMON_OPT_FILE macro (except of course the one oddball - opts_screenshot in domain_conf Tks - John
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@redhat.com> --- tools/virsh-volume.c | 87 ++++++++++++++++------------------------------------ 1 file changed, 27 insertions(+), 60 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 7932ef2..5d9cade 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -42,6 +42,19 @@ #include "virxml.h" #include "virstring.h" +#define VIRSH_COMMON_OPT_VOLUME_POOL_CREATE \ + {.name = "pool", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("pool name") \ + } \ + +#define VIRSH_COMMON_OPT_VOLUME_POOL_STRING \ + {.name = "pool", \ + .type = VSH_OT_STRING, \ + .help = N_("pool name or uuid") \ + } \ + virStorageVolPtr virshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -145,11 +158,7 @@ static const vshCmdInfo info_vol_create_as[] = { }; static const vshCmdOptDef opts_vol_create_as[] = { - {.name = "pool", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("pool name") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_CREATE, {.name = "name", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -353,11 +362,7 @@ static const vshCmdInfo info_vol_create[] = { }; static const vshCmdOptDef opts_vol_create[] = { - {.name = "pool", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("pool name") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_CREATE, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -423,11 +428,7 @@ static const vshCmdInfo info_vol_create_from[] = { }; static const vshCmdOptDef opts_vol_create_from[] = { - {.name = "pool", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_POOL, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -558,10 +559,7 @@ static const vshCmdOptDef opts_vol_clone[] = { .flags = VSH_OFLAG_REQ, .help = N_("clone name") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "prealloc-metadata", .type = VSH_OT_BOOL, .help = N_("preallocate metadata (for qcow2 instead of full allocation)") @@ -661,10 +659,7 @@ static const vshCmdOptDef opts_vol_upload[] = { .flags = VSH_OFLAG_REQ, .help = N_("file") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "offset", .type = VSH_OT_INT, .help = N_("volume offset to upload to") @@ -775,10 +770,7 @@ static const vshCmdOptDef opts_vol_download[] = { .flags = VSH_OFLAG_REQ, .help = N_("file") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "offset", .type = VSH_OT_INT, .help = N_("volume offset to download from") @@ -883,10 +875,7 @@ static const vshCmdOptDef opts_vol_delete[] = { .flags = VSH_OFLAG_REQ, .help = N_("vol name, key or path") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "delete-snapshots", .type = VSH_OT_BOOL, .help = N_("delete snapshots associated with volume (must be " @@ -940,10 +929,7 @@ static const vshCmdOptDef opts_vol_wipe[] = { .flags = VSH_OFLAG_REQ, .help = N_("vol name, key or path") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "algorithm", .type = VSH_OT_STRING, .help = N_("perform selected wiping algorithm") @@ -1033,10 +1019,7 @@ static const vshCmdOptDef opts_vol_info[] = { .flags = VSH_OFLAG_REQ, .help = N_("vol name, key or path") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; @@ -1096,10 +1079,7 @@ static const vshCmdOptDef opts_vol_resize[] = { .flags = VSH_OFLAG_REQ, .help = N_("new capacity for the vol, as scaled integer (default bytes)") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "allocate", .type = VSH_OT_BOOL, .help = N_("allocate the new capacity, rather than leaving it sparse") @@ -1195,10 +1175,7 @@ static const vshCmdOptDef opts_vol_dumpxml[] = { .flags = VSH_OFLAG_REQ, .help = N_("vol name, key or path") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; @@ -1364,11 +1341,7 @@ static const vshCmdInfo info_vol_list[] = { }; static const vshCmdOptDef opts_vol_list[] = { - {.name = "pool", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_POOL, {.name = "details", .type = VSH_OT_BOOL, .help = N_("display extended details for volumes") @@ -1710,10 +1683,7 @@ static const vshCmdOptDef opts_vol_key[] = { .flags = VSH_OFLAG_REQ, .help = N_("volume name or path") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; @@ -1749,10 +1719,7 @@ static const vshCmdOptDef opts_vol_path[] = { .flags = VSH_OFLAG_REQ, .help = N_("volume name or key") }, - {.name = "pool", - .type = VSH_OT_STRING, - .help = N_("pool name or uuid") - }, + VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; -- 2.5.0
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@redhat.com> --- tools/virsh-volume.c | 87 ++++++++++++++++------------------------------------ 1 file changed, 27 insertions(+), 60 deletions(-)
I really don't like this :) 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. 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? 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
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@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
On Mon, 2016-01-11 at 18:03 -0500, John Ferlan wrote:
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.
What's the advantage of doing so? Now we have two separate, identical definitions for eg. VIRSH_COMMON_OPT_DOMAIN_FULL...
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.
Right. I always forget that :( Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Mon, Jan 11, 2016 at 04:39:25PM +0100, 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@redhat.com> --- tools/virsh-volume.c | 87 ++++++++++++++++------------------------------------ 1 file changed, 27 insertions(+), 60 deletions(-)
I really don't like this :)
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.
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?
No. Jan
On Tue, 2016-01-12 at 09:28 +0100, Ján Tomko wrote:
+#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.
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?
No.
Don't worry, John went with VIRSH_COMMON_OPT_POOL_NAME after all. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create common macros to be used generically. Replace the more commonly used "vol" option with a macro. This also adjusts 2 commands that didn't have the correct helpstr - 'vol-create-from' and 'vol-clone'. Both are described in the man page as taking vol, path, or key and the code uses the virshCommandOptVol instead of virshCommandOptVolBy. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 61 ++++++++++++++-------------------------------------- 1 file changed, 16 insertions(+), 45 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 5d9cade..58bfe03 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -55,6 +55,13 @@ .help = N_("pool name or uuid") \ } \ +#define VIRSH_COMMON_OPT_VOLUME_VOL \ + {.name = "vol", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("vol name, key or path") \ + } \ + virStorageVolPtr virshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, @@ -434,11 +441,7 @@ static const vshCmdOptDef opts_vol_create_from[] = { .flags = VSH_OFLAG_REQ, .help = N_("file containing an XML vol description") }, - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("input vol name or key") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, {.name = "inputpool", .type = VSH_OT_STRING, .help = N_("pool name or uuid of the input volume's pool") @@ -549,11 +552,7 @@ static const vshCmdInfo info_vol_clone[] = { }; static const vshCmdOptDef opts_vol_clone[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("orig vol name or key") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, {.name = "newname", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -649,11 +648,7 @@ static const vshCmdInfo info_vol_upload[] = { }; static const vshCmdOptDef opts_vol_upload[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -760,11 +755,7 @@ static const vshCmdInfo info_vol_download[] = { }; static const vshCmdOptDef opts_vol_download[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, {.name = "file", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -870,11 +861,7 @@ static const vshCmdInfo info_vol_delete[] = { }; static const vshCmdOptDef opts_vol_delete[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "delete-snapshots", .type = VSH_OT_BOOL, @@ -924,11 +911,7 @@ static const vshCmdInfo info_vol_wipe[] = { }; static const vshCmdOptDef opts_vol_wipe[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = "algorithm", .type = VSH_OT_STRING, @@ -1014,11 +997,7 @@ static const vshCmdInfo info_vol_info[] = { }; static const vshCmdOptDef opts_vol_info[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; @@ -1069,11 +1048,7 @@ static const vshCmdInfo info_vol_resize[] = { }; static const vshCmdOptDef opts_vol_resize[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, {.name = "capacity", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1170,11 +1145,7 @@ static const vshCmdInfo info_vol_dumpxml[] = { }; static const vshCmdOptDef opts_vol_dumpxml[] = { - {.name = "vol", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("vol name, key or path") - }, + VIRSH_COMMON_OPT_VOLUME_VOL, VIRSH_COMMON_OPT_VOLUME_POOL_STRING, {.name = NULL} }; -- 2.5.0
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. Replace the more commonly used "vol" option with a macro. This also adjusts 2 commands that didn't have the correct helpstr - 'vol-create-from' and 'vol-clone'. Both are described in the man page as taking vol, path, or key and the code uses the virshCommandOptVol instead of virshCommandOptVolBy. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-volume.c | 61 ++++++++++++++-------------------------------------- 1 file changed, 16 insertions(+), 45 deletions(-)
I'm a bit conflicted about the macro name - I could maybe see VIRSH_COMMON_OPT_VOLUME as an alternative, but even that would not be completely satisfactory. I guess there's no clear winner in this kind of situation, so let's not worry too much about it. We can always change it whenever :) ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
Make use of the common VIRSH_COMMON_OPT_DOMAIN macro for each of the matching '.name = "domain"' command options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain-monitor.c | 77 ++++++++------------------------------------ 1 file changed, 13 insertions(+), 64 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 7dcf833..6efb5c4 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -281,11 +281,7 @@ static const vshCmdInfo info_dommemstat[] = { }; static const vshCmdOptDef opts_dommemstat[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "period", .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ_OPT, @@ -402,11 +398,7 @@ static const vshCmdInfo info_domblkinfo[] = { }; static const vshCmdOptDef opts_domblkinfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -457,11 +449,7 @@ static const vshCmdInfo info_domblklist[] = { }; static const vshCmdOptDef opts_domblklist[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("get inactive rather than running configuration") @@ -579,11 +567,7 @@ static const vshCmdInfo info_domiflist[] = { }; static const vshCmdOptDef opts_domiflist[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("get inactive rather than running configuration") @@ -684,11 +668,7 @@ static const vshCmdInfo info_domif_getlink[] = { }; static const vshCmdOptDef opts_domif_getlink[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "interface", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -799,11 +779,7 @@ static const vshCmdInfo info_domcontrol[] = { }; static const vshCmdOptDef opts_domcontrol[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -856,11 +832,7 @@ static const vshCmdInfo info_domblkstat[] = { }; static const vshCmdOptDef opts_domblkstat[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "device", .type = VSH_OT_STRING, .flags = VSH_OFLAG_EMPTY_OK, @@ -1046,11 +1018,7 @@ static const vshCmdInfo info_domifstat[] = { }; static const vshCmdOptDef opts_domifstat[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "interface", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1123,11 +1091,7 @@ static const vshCmdInfo info_domblkerror[] = { }; static const vshCmdOptDef opts_domblkerror[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id, or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -1188,11 +1152,7 @@ static const vshCmdInfo info_dominfo[] = { }; static const vshCmdOptDef opts_dominfo[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = NULL} }; @@ -1331,11 +1291,7 @@ static const vshCmdInfo info_domstate[] = { }; static const vshCmdOptDef opts_domstate[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "reason", .type = VSH_OT_BOOL, .help = N_("also print reason for the state") @@ -1387,11 +1343,7 @@ static const vshCmdInfo info_domtime[] = { }; static const vshCmdOptDef opts_domtime[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "now", .type = VSH_OT_BOOL, .help = N_("set to the time of the host running virsh") @@ -2208,10 +2160,7 @@ static const vshCmdInfo info_domifaddr[] = { }; static const vshCmdOptDef opts_domifaddr[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid")}, + VIRSH_COMMON_OPT_DOMAIN, {.name = "interface", .type = VSH_OT_STRING, .flags = VSH_OFLAG_NONE, -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Make use of the common VIRSH_COMMON_OPT_DOMAIN macro for each of the matching '.name = "domain"' command options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain-monitor.c | 77 ++++++++------------------------------------ 1 file changed, 13 insertions(+), 64 deletions(-)
Any reason for this and 12/14 not to be squashed into 03/14? At the very least, I'd expect them to be placed right after 03/14 in the series. ACK to the changes themselves. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 01/11/2016 08:07 AM, Andrea Bolognani wrote:
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Make use of the common VIRSH_COMMON_OPT_DOMAIN macro for each of the matching '.name = "domain"' command options.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain-monitor.c | 77 ++++++++------------------------------------ 1 file changed, 13 insertions(+), 64 deletions(-)
Any reason for this and 12/14 not to be squashed into 03/14?
Because these two were done after the original series and they're a separate source file
At the very least, I'd expect them to be placed right after 03/14 in the series.
I can squash - that's fine. I think separation is perhaps more appropriate for "review purposes", but functionally sp Tks - John
ACK to the changes themselves.
Cheers.
-- Andrea Bolognani Software Engineer - Virtualization Team
Make use of the common VIRSH_COMMON_OPT_DOMAIN macro for each of the matching '.name = "domain"' command options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-snapshot.c | 60 +++++++++----------------------------------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 3ab2104..b806d7c 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -123,11 +123,7 @@ static const vshCmdInfo info_snapshot_create[] = { }; static const vshCmdOptDef opts_snapshot_create[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "xmlfile", .type = VSH_OT_STRING, .help = N_("domain snapshot XML") @@ -328,11 +324,7 @@ static const vshCmdInfo info_snapshot_create_as[] = { }; static const vshCmdOptDef opts_snapshot_create_as[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "name", .type = VSH_OT_STRING, .help = N_("name of snapshot") @@ -524,11 +516,7 @@ static const vshCmdInfo info_snapshot_edit[] = { }; static const vshCmdOptDef opts_snapshot_edit[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_STRING, .help = N_("snapshot name") @@ -645,11 +633,7 @@ static const vshCmdInfo info_snapshot_current[] = { }; static const vshCmdOptDef opts_snapshot_current[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "name", .type = VSH_OT_BOOL, .help = N_("list the name, rather than the full xml") @@ -882,11 +866,7 @@ static const vshCmdInfo info_snapshot_info[] = { }; static const vshCmdOptDef opts_snapshot_info[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_STRING, .help = N_("snapshot name") @@ -1441,11 +1421,7 @@ static const vshCmdInfo info_snapshot_list[] = { }; static const vshCmdOptDef opts_snapshot_list[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "parent", .type = VSH_OT_BOOL, .help = N_("add a column showing parent snapshot") @@ -1705,11 +1681,7 @@ static const vshCmdInfo info_snapshot_dumpxml[] = { }; static const vshCmdOptDef opts_snapshot_dumpxml[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1773,11 +1745,7 @@ static const vshCmdInfo info_snapshot_parent[] = { }; static const vshCmdOptDef opts_snapshot_parent[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_STRING, .help = N_("find parent of snapshot name") @@ -1841,11 +1809,7 @@ static const vshCmdInfo info_snapshot_revert[] = { }; static const vshCmdOptDef opts_snapshot_revert[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_STRING, .help = N_("snapshot name") @@ -1934,11 +1898,7 @@ static const vshCmdInfo info_snapshot_delete[] = { }; static const vshCmdOptDef opts_snapshot_delete[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN, {.name = "snapshotname", .type = VSH_OT_STRING, .help = N_("snapshot name") -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Make use of the common VIRSH_COMMON_OPT_DOMAIN macro for each of the matching '.name = "domain"' command options.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-snapshot.c | 60 +++++++++----------------------------------------- 1 file changed, 10 insertions(+), 50 deletions(-)
See comments for 11/14. ACK to the changes themselves. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "network",' entries are replaced, just those that have the common .help string of "network name or uuid". Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-network.c | 61 ++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 45 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index bd89c11..c13acd4 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -33,6 +33,13 @@ #include "virstring.h" #include "conf/network_conf.h" +#define VIRSH_COMMON_OPT_NETWORK \ + {.name = "network", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("network name or uuid") \ + } \ + virNetworkPtr virshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, const char **name, unsigned int flags) @@ -85,11 +92,7 @@ static const vshCmdInfo info_network_autostart[] = { }; static const vshCmdOptDef opts_network_autostart[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = "disable", .type = VSH_OT_BOOL, .help = N_("disable autostarting") @@ -244,11 +247,7 @@ static const vshCmdInfo info_network_destroy[] = { }; static const vshCmdOptDef opts_network_destroy[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = NULL} }; @@ -287,11 +286,7 @@ static const vshCmdInfo info_network_dumpxml[] = { }; static const vshCmdOptDef opts_network_dumpxml[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("network information of an inactive domain") @@ -342,11 +337,7 @@ static const vshCmdInfo info_network_info[] = { }; static const vshCmdOptDef opts_network_info[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = NULL} }; @@ -795,11 +786,7 @@ static const vshCmdInfo info_network_start[] = { }; static const vshCmdOptDef opts_network_start[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = NULL} }; @@ -837,11 +824,7 @@ static const vshCmdInfo info_network_undefine[] = { }; static const vshCmdOptDef opts_network_undefine[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = NULL} }; @@ -880,11 +863,7 @@ static const vshCmdInfo info_network_update[] = { }; static const vshCmdOptDef opts_network_update[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = "command", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -1097,11 +1076,7 @@ static const vshCmdInfo info_network_edit[] = { }; static const vshCmdOptDef opts_network_edit[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = NULL} }; @@ -1329,11 +1304,7 @@ static const vshCmdInfo info_network_dhcp_leases[] = { }; static const vshCmdOptDef opts_network_dhcp_leases[] = { - {.name = "network", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("network name or uuid") - }, + VIRSH_COMMON_OPT_NETWORK, {.name = "mac", .type = VSH_OT_STRING, .flags = VSH_OFLAG_NONE, -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "network",' entries are replaced, just those that have the common .help string of "network name or uuid".
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-network.c | 61 ++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 45 deletions(-)
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "interface",' entries are replaced, just those that have the common .help string of "interface name or MAC address". Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-interface.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index b69c685..0613c76 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -23,6 +23,13 @@ * */ +#define VIRSH_COMMON_OPT_INTERFACE \ + {.name = "interface", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("interface name or MAC address") \ + } \ + #include <config.h> #include "virsh-interface.h" @@ -100,11 +107,7 @@ static const vshCmdInfo info_interface_edit[] = { }; static const vshCmdOptDef opts_interface_edit[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name or MAC address") - }, + VIRSH_COMMON_OPT_INTERFACE, {.name = NULL} }; @@ -464,11 +467,7 @@ static const vshCmdInfo info_interface_dumpxml[] = { }; static const vshCmdOptDef opts_interface_dumpxml[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name or MAC address") - }, + VIRSH_COMMON_OPT_INTERFACE, {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("show inactive defined XML") @@ -569,11 +568,7 @@ static const vshCmdInfo info_interface_undefine[] = { }; static const vshCmdOptDef opts_interface_undefine[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name or MAC address") - }, + VIRSH_COMMON_OPT_INTERFACE, {.name = NULL} }; @@ -612,11 +607,7 @@ static const vshCmdInfo info_interface_start[] = { }; static const vshCmdOptDef opts_interface_start[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name or MAC address") - }, + VIRSH_COMMON_OPT_INTERFACE, {.name = NULL} }; @@ -655,11 +646,7 @@ static const vshCmdInfo info_interface_destroy[] = { }; static const vshCmdOptDef opts_interface_destroy[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name or MAC address") - }, + VIRSH_COMMON_OPT_INTERFACE, {.name = NULL} }; -- 2.5.0
On Sat, 2016-01-09 at 08:36 -0500, John Ferlan wrote:
Rather than continually cut-n-paste the strings into each command, create a common macro to be used generically. Note that not all '{.name = "interface",' entries are replaced, just those that have the common .help string of "interface name or MAC address".
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-interface.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-)
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
On 01/09/2016 08:36 AM, John Ferlan wrote:
v2: http://www.redhat.com/archives/libvir-list/2015-December/msg00766.html
Changes since v2:
Use VIRSH_COMMON_OPT_<optname> for option prefix instead of VIRSH_<optname>_OPT_COMMON
Patches have a few thumbs up already, figured I'd post it one last time for perusal and checks of the naming 'algorithm.
John Ferlan (14): virsh: Covert VSH_POOL_ macro to VIRSH_COMMON_OPT_ virsh: Move VIRSH_COMMON_OPT_POOL to virsh.h virsh: Create macro for common "domain" option virsh: Create macro for common "persistent" option virsh: Create macro for common "config" option virsh: Create macro for common "live" option virsh: Create macro for common "current" option virsh: Create macro for common "file" option virsh: Create macros for common "pool" options virsh: Create macros for common "vol" options virsh: Have domain-monitor use common "domain" option virsh: have snapshot use common "domain" option virsh: Create macro for common "network" option virsh: Create macro for common "interface" option
po/POTFILES.in | 1 + tools/virsh-domain-monitor.c | 77 +--- tools/virsh-domain.c | 911 +++++++++---------------------------------- tools/virsh-interface.c | 37 +- tools/virsh-network.c | 61 +-- tools/virsh-pool.c | 71 ++-- tools/virsh-snapshot.c | 60 +-- tools/virsh-volume.c | 148 ++----- tools/virsh.h | 17 + 9 files changed, 334 insertions(+), 1049 deletions(-)
After performing all the requested fixups, I've now pushed the entire series. Thanks for taking the time to review - I know it was tedius. John
participants (4)
-
Andrea Bolognani -
John Ferlan -
Ján Tomko -
Pavel Hrdina