[libvirt] [PATCH v2 00/12] Some fixes and improvement for virsh completion in non interactive mode
This patch series is about some fixes and improvement for virsh completion in non interactive mode. Some of them probably don't make sense, I sent them out for suggestions. v2->v1: * Add a new patch for centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_FULL to virsh.h * rename the helper function from virDomainEventGetName virshDomainEventGetName Follow Peter Krempa's suggestion: * Drop the original patch#10 which about introduce some VIR_DOMAIN_EVENT_* macros * code formatting fix * centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL * centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_OT_ARGV_FULL * refactor some code in patch#10 for using same array, handle the case when '--event' and '--all' are provided together through command line. * Due to the helper function is only used for virsh, unnecessary to be publiced, I move it from src/conf/domain_event.c to tools/virsh-util.c Lin Ma (12): virsh: Move the definition of macro VIRSH_COMMON_OPT_DOMAIN_FULL to virsh.h virsh: Add domain name completion to 'migrate-postcopy' command virsh: Conditionally Ignore the first entry in list of completions virsh: Create macros for VSH_OT_STRING "domain" option virsh: Apply macro for current VSH_OT_STRING "domain" options vshReadlineParse: Ignore vshReadlineOptionsGenerator for VSH_OT_ARGV options virsh: Create macros for VSH_OT_ARGV "domain" option virsh: Apply macro for current VSH_OT_ARGV "domain" options vshReadlineOptionsGenerator: Add already provided VSH_OT_ARGV options to list virsh: Enable multiple --event flags to 'event' command virsh: add helper for returning event name string virsh: Add event name completion to 'event' command tools/virsh-completer.c | 37 ++++++++++++ tools/virsh-completer.h | 3 + tools/virsh-domain-monitor.c | 9 +-- tools/virsh-domain.c | 135 ++++++++++++++++++++----------------------- tools/virsh-snapshot.c | 3 - tools/virsh-util.c | 60 +++++++++++++++++++ tools/virsh-util.h | 3 + tools/virsh.h | 26 +++++++++ tools/vsh.c | 12 +++- 9 files changed, 203 insertions(+), 85 deletions(-) -- 2.15.1
centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_FULL to virsh.h to avoid unnecessary duplicated definition Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain-monitor.c | 3 --- tools/virsh-domain.c | 3 --- tools/virsh-snapshot.c | 3 --- tools/virsh.h | 3 +++ 4 files changed, 3 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8e071779b4..071619d0e3 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -40,9 +40,6 @@ #include "virxml.h" #include "virstring.h" -#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - VIR_ENUM_DECL(virshDomainIOError) VIR_ENUM_IMPL(virshDomainIOError, VIR_DOMAIN_DISK_ERROR_LAST, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 598d2fa4a4..aa11a81638 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -65,9 +65,6 @@ # define SA_SIGINFO 0 #endif -#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - #define VIRSH_COMMON_OPT_DOMAIN_PERSISTENT \ {.name = "persistent", \ .type = VSH_OT_BOOL, \ diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e4908eea70..812fa91333 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -42,9 +42,6 @@ #include "virxml.h" #include "conf/snapshot_conf.h" -#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - /* Helper for snapshot-create and snapshot-create-as */ static bool virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, diff --git a/tools/virsh.h b/tools/virsh.h index f2213ebb57..9e717ef574 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -82,6 +82,9 @@ .completer_flags = cflags, \ } +#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ + VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) + # define VIRSH_COMMON_OPT_CONFIG(_helpstr) \ {.name = "config", \ .type = VSH_OT_BOOL, \ -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
centralize the definition of macro VIRSH_COMMON_OPT_DOMAIN_FULL to virsh.h to avoid unnecessary duplicated definition
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain-monitor.c | 3 --- tools/virsh-domain.c | 3 --- tools/virsh-snapshot.c | 3 --- tools/virsh.h | 3 +++ 4 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8e071779b4..071619d0e3 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -40,9 +40,6 @@ #include "virxml.h" #include "virstring.h"
-#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - VIR_ENUM_DECL(virshDomainIOError) VIR_ENUM_IMPL(virshDomainIOError, VIR_DOMAIN_DISK_ERROR_LAST, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 598d2fa4a4..aa11a81638 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -65,9 +65,6 @@ # define SA_SIGINFO 0 #endif
-#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - #define VIRSH_COMMON_OPT_DOMAIN_PERSISTENT \ {.name = "persistent", \ .type = VSH_OT_BOOL, \ diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e4908eea70..812fa91333 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -42,9 +42,6 @@ #include "virxml.h" #include "conf/snapshot_conf.h"
-#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ - VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags) - /* Helper for snapshot-create and snapshot-create-as */ static bool virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, diff --git a/tools/virsh.h b/tools/virsh.h index f2213ebb57..9e717ef574 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -82,6 +82,9 @@ .completer_flags = cflags, \ }
+#define VIRSH_COMMON_OPT_DOMAIN_FULL(cflags) \ + VIRSH_COMMON_OPT_DOMAIN(N_("domain name, id or uuid"), cflags)
This needs to be: # define VIRSH_... because it's nested #define. syntax-check would catch this. ACK with that fixed. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index aa11a81638..ace5c02871 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11213,11 +11213,7 @@ static const vshCmdInfo info_migrate_postcopy[] = { }; static const vshCmdOptDef opts_migrate_postcopy[] = { - {.name = "domain", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = NULL} }; -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
ACK Michal
The first entry in the returned array is the substitution for TEXT. It causes unnecessary output if other commands or options share the same prefix, e.g. $ virsh des<TAB><TAB> des desc destroy or $ virsh domblklist --d<TAB><TAB> --d --details --domain This patch fixes the above issue. Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 73ec007e56..57f7589b53 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) const vshCmdOpt *opt = NULL; char **matches = NULL, **iter; virBuffer buf = VIR_BUFFER_INITIALIZER; + int n; if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) goto cleanup; @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) if (!(matches = vshReadlineCompletion(arg, 0, 0))) goto cleanup; - for (iter = matches; *iter; iter++) + for (n = 0, iter = matches; *iter; iter++, n++) { + if (n == 0 && matches[1]) + continue; printf("%s\n", *iter); + } ret = true; cleanup: -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
The first entry in the returned array is the substitution for TEXT. It causes unnecessary output if other commands or options share the same prefix, e.g.
$ virsh des<TAB><TAB> des desc destroy
or
$ virsh domblklist --d<TAB><TAB> --d --details --domain
This patch fixes the above issue.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 73ec007e56..57f7589b53 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) const vshCmdOpt *opt = NULL; char **matches = NULL, **iter; virBuffer buf = VIR_BUFFER_INITIALIZER; + int n;
This needs to be size_t. Even though it's not used to directly access entries of @matches array, it kind of is. It's used to count them. And int is not guaranteed to be able to address all of them.
if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) goto cleanup; @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) if (!(matches = vshReadlineCompletion(arg, 0, 0))) goto cleanup;
- for (iter = matches; *iter; iter++) + for (n = 0, iter = matches; *iter; iter++, n++) { + if (n == 0 && matches[1]) + continue;
This can be rewritten so that we don't need @n at all: if (iter == matches && matches[1]) continue; Or even better, just start iterating not from the first but second entry: for (iter = &matches[1]; *iter; iter++) printf("%s\n", *iter); This is safe because: a) @matches is guaranteed to be non-NULL at this point (due to check above), b) @matches is NULL terminated array and as you and documentation [1] say, the first entry is substitution for text (which we want to skip). So even if the array is nothing but the substitution and NULL, matches[1] will be NULL: matches = { "subs", NULL }; Also, I'm adding a small comment because it is not obvious why we are skipping the first entry. ACK then. Michal 1: http://www.delorie.com/gnu/docs/readline/rlman_46.html
On 05/08/2018 04:20 PM, Lin Ma wrote:
The first entry in the returned array is the substitution for TEXT. It causes unnecessary output if other commands or options share the same prefix, e.g.
$ virsh des<TAB><TAB> des desc destroy
or
$ virsh domblklist --d<TAB><TAB> --d --details --domain
This patch fixes the above issue.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 73ec007e56..57f7589b53 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) const vshCmdOpt *opt = NULL; char **matches = NULL, **iter; virBuffer buf = VIR_BUFFER_INITIALIZER; + int n; This needs to be size_t. Even though it's not used to directly access entries of @matches array, it kind of is. It's used to count them. And int is not guaranteed to be able to address all of them.
if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) goto cleanup; @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) if (!(matches = vshReadlineCompletion(arg, 0, 0))) goto cleanup;
- for (iter = matches; *iter; iter++) + for (n = 0, iter = matches; *iter; iter++, n++) { + if (n == 0 && matches[1]) + continue;
This can be rewritten so that we don't need @n at all:
if (iter == matches && matches[1]) continue;
Or even better, just start iterating not from the first but second entry:
for (iter = &matches[1]; *iter; iter++) printf("%s\n", *iter); It seems it can't handle the case that no command or option share the same prefix. say 'event' command, when users type virsh ev<TAB><TAB>, there is no other command sharing 'ev' prefix, in this case, the matches[1] is NULL and we need to
On 05/10/2018 05:17 PM, Michal Privoznik wrote: print the value in matches[0],I think we can't skip the first entry in this case. So the code block 'if (iter == matches && matches[1]) continue;' looks like a better choice.If you think so, I'd like to write a patch to do it, Do you agree? meanwhile, I want to remove those comment due to we can't skip first entry, Do you agree?
This is safe because: a) @matches is guaranteed to be non-NULL at this point (due to check above), b) @matches is NULL terminated array and as you and documentation [1] say, the first entry is substitution for text (which we want to skip).
So even if the array is nothing but the substitution and NULL, matches[1] will be NULL:
matches = { "subs", NULL };
Also, I'm adding a small comment because it is not obvious why we are skipping the first entry.
ACK then.
Michal
Thanks, Lin
On 05/11/2018 10:01 AM, Lin Ma wrote:
On 05/08/2018 04:20 PM, Lin Ma wrote:
The first entry in the returned array is the substitution for TEXT. It causes unnecessary output if other commands or options share the same prefix, e.g.
$ virsh des<TAB><TAB> des desc destroy
or
$ virsh domblklist --d<TAB><TAB> --d --details --domain
This patch fixes the above issue.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 73ec007e56..57f7589b53 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) const vshCmdOpt *opt = NULL; char **matches = NULL, **iter; virBuffer buf = VIR_BUFFER_INITIALIZER; + int n; This needs to be size_t. Even though it's not used to directly access entries of @matches array, it kind of is. It's used to count them. And int is not guaranteed to be able to address all of them.
if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) goto cleanup; @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) if (!(matches = vshReadlineCompletion(arg, 0, 0))) goto cleanup; - for (iter = matches; *iter; iter++) + for (n = 0, iter = matches; *iter; iter++, n++) { + if (n == 0 && matches[1]) + continue; This can be rewritten so that we don't need @n at all:
if (iter == matches && matches[1]) continue;
Or even better, just start iterating not from the first but second entry:
for (iter = &matches[1]; *iter; iter++) printf("%s\n", *iter); It seems it can't handle the case that no command or option share the same prefix. say 'event' command, when users type virsh ev<TAB><TAB>, there is no other command sharing 'ev' prefix, in this case, the matches[1] is NULL and we need to
On 05/10/2018 05:17 PM, Michal Privoznik wrote: print the value in matches[0],I think we can't skip the first entry in this case. So the code block 'if (iter == matches && matches[1]) continue;' looks like a better choice.If you think so, I'd like to write a patch to do it, Do you agree? meanwhile, I want to remove those comment due to we can't skip first entry, Do you agree?
Ah good catch. You're right, the documentation is not valid then. Sure, if you can write the patch I'm happy to review it. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tools/virsh.h b/tools/virsh.h index 9e717ef574..b1b641bc41 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -110,6 +110,17 @@ .help = _helpstr \ } +# define VIRSH_COMMON_OPT_DOMAIN_OT_STRING(_helpstr, cflags) \ + {.name = "domain", \ + .type = VSH_OT_STRING, \ + .help = _helpstr, \ + .completer = virshDomainNameCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(cflags) \ + VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("domain name, id or uuid"), cflags) + typedef struct _virshControl virshControl; typedef virshControl *virshControlPtr; -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh.h b/tools/virsh.h index 9e717ef574..b1b641bc41 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -110,6 +110,17 @@ .help = _helpstr \ }
+# define VIRSH_COMMON_OPT_DOMAIN_OT_STRING(_helpstr, cflags) \ + {.name = "domain", \ + .type = VSH_OT_STRING, \ + .help = _helpstr, \ + .completer = virshDomainNameCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(cflags) \ + VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("domain name, id or uuid"), cflags)
Again, missing space after #. ACK with that fixed. Michal
These VSH_OT_STRING "domain" options support domain name completion now. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ace5c02871..30da953446 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9592,10 +9592,8 @@ static const vshCmdInfo info_qemu_monitor_event[] = { }; static const vshCmdOptDef opts_qemu_monitor_event[] = { - {.name = "domain", - .type = VSH_OT_STRING, - .help = N_("filter by domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), + 0), {.name = "event", .type = VSH_OT_STRING, .help = N_("filter by event name") @@ -10148,11 +10146,7 @@ static const vshCmdOptDef opts_domxmltonative[] = { .flags = VSH_OFLAG_REQ, .help = N_("target config data type format") }, - {.name = "domain", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, - .help = N_("domain name, id or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(0), {.name = "xml", .type = VSH_OT_STRING, .help = N_("xml data file to export from") @@ -13339,10 +13333,8 @@ static const vshCmdInfo info_event[] = { }; static const vshCmdOptDef opts_event[] = { - {.name = "domain", - .type = VSH_OT_STRING, - .help = N_("filter by domain name, id, or uuid") - }, + VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), + 0), {.name = "event", .type = VSH_OT_STRING, .help = N_("which event type to wait for") -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
These VSH_OT_STRING "domain" options support domain name completion now.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
ACK Michal
Currently the VSH_OT_ARGV options don't support complete, But some of VSH_OT_ARGV options are gonna support complete in upcoming patches. Once applied the upcoming completion patches for VSH_OT_ARGV options, If we don't ignore VSH_OT_ARGV here, The vshReadlineOptionsGenerator will be called, Hence complete output will consist of the result by command completer + the result by option completer, It's confusing. e.g. $ virsh domstats --domain <TAB><TAB> --backing --interface --list-paused --perf --vcpu --balloon leap42.3 --list-persistent --raw win10 --block --list-active --list-running sles12sp3 --cpu-total --list-inactive --list-shutoff sles15 --enforce --list-other --list-transient --state After this patch and the upcoming completion patches: $ virsh domstats --domain <TAB><TAB> leap42.3 sles12sp3 sles15 win10 Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 57f7589b53..e45bb0d825 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2824,7 +2824,9 @@ vshReadlineParse(const char *text, int state) if (!cmd) { list = vshReadlineCommandGenerator(text); } else { - if (!opt || (opt->type != VSH_OT_DATA && opt->type != VSH_OT_STRING)) + if (!opt || (opt->type != VSH_OT_DATA && + opt->type != VSH_OT_STRING && + opt->type != VSH_OT_ARGV)) list = vshReadlineOptionsGenerator(text, cmd, partial); if (opt && opt->completer) { -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Currently the VSH_OT_ARGV options don't support complete, But some of VSH_OT_ARGV options are gonna support complete in upcoming patches.
Once applied the upcoming completion patches for VSH_OT_ARGV options, If we don't ignore VSH_OT_ARGV here, The vshReadlineOptionsGenerator will be called, Hence complete output will consist of the result by command completer + the result by option completer, It's confusing. e.g. $ virsh domstats --domain <TAB><TAB> --backing --interface --list-paused --perf --vcpu --balloon leap42.3 --list-persistent --raw win10 --block --list-active --list-running sles12sp3 --cpu-total --list-inactive --list-shutoff sles15 --enforce --list-other --list-transient --state
After this patch and the upcoming completion patches: $ virsh domstats --domain <TAB><TAB> leap42.3 sles12sp3 sles15 win10
Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index 57f7589b53..e45bb0d825 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2824,7 +2824,9 @@ vshReadlineParse(const char *text, int state) if (!cmd) { list = vshReadlineCommandGenerator(text); } else { - if (!opt || (opt->type != VSH_OT_DATA && opt->type != VSH_OT_STRING)) + if (!opt || (opt->type != VSH_OT_DATA && + opt->type != VSH_OT_STRING && + opt->type != VSH_OT_ARGV)) list = vshReadlineOptionsGenerator(text, cmd, partial);
if (opt && opt->completer) {
I don't think this is right. The VSH_OT_ARGV means that there can be more than one argument. And these are usually specified at the end like this: # virsh domstats dom1 dom2 dom3 This patch allows to specify just one domain. I guess we need a specific completer for this command. After going further with the review I had revisit this patch again. Now it makes perfect sense, because after patch 09/12 you allow multiple --domain arguments. Therefore I think a small reorder is in place. 09/12 should be placed before this patch. I'll fix that when pushing. ACK Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tools/virsh.h b/tools/virsh.h index b1b641bc41..4353ff46d4 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -121,6 +121,18 @@ #define VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(cflags) \ VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("domain name, id or uuid"), cflags) +# define VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(_helpstr, cflags) \ + {.name = "domain", \ + .type = VSH_OT_ARGV, \ + .flags = VSH_OFLAG_NONE, \ + .help = _helpstr, \ + .completer = virshDomainNameCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_COMMON_OPT_DOMAIN_OT_ARGV_FULL(cflags) \ + VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(N_("domain name, id or uuid"), cflags) + typedef struct _virshControl virshControl; typedef virshControl *virshControlPtr; -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/tools/virsh.h b/tools/virsh.h index b1b641bc41..4353ff46d4 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -121,6 +121,18 @@ #define VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(cflags) \ VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("domain name, id or uuid"), cflags)
+# define VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(_helpstr, cflags) \ + {.name = "domain", \ + .type = VSH_OT_ARGV, \ + .flags = VSH_OFLAG_NONE, \ + .help = _helpstr, \ + .completer = virshDomainNameCompleter, \> + .completer_flags = cflags, \ + } + +#define VIRSH_COMMON_OPT_DOMAIN_OT_ARGV_FULL(cflags) \
Again, missing space. ACK with that fixed. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain-monitor.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 071619d0e3..fa93f3a312 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1991,11 +1991,7 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("add backing chain information to block stats"), }, - {.name = "domain", - .type = VSH_OT_ARGV, - .flags = VSH_OFLAG_NONE, - .help = N_("list of domains to get stats for"), - }, + VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(N_("list of domains to get stats for"), 0), {.name = NULL} }; -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain-monitor.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
ACK Michal
It's helpful for users while they type certain kind of VSH_OT_ARGV options. e.g. $ virsh domstats --domain sles12sp3 --d<TAB> Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index e45bb0d825..279d1b56e6 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2685,7 +2685,7 @@ vshReadlineOptionsGenerator(const char *text, } while (opt) { - if (STREQ(opt->def->name, name)) { + if (STREQ(opt->def->name, name) && opt->def->type != VSH_OT_ARGV) { exists = true; break; } -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
It's helpful for users while they type certain kind of VSH_OT_ARGV options. e.g.
$ virsh domstats --domain sles12sp3 --d<TAB>
Signed-off-by: Lin Ma <lma@suse.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c index e45bb0d825..279d1b56e6 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2685,7 +2685,7 @@ vshReadlineOptionsGenerator(const char *text, }
while (opt) { - if (STREQ(opt->def->name, name)) { + if (STREQ(opt->def->name, name) && opt->def->type != VSH_OT_ARGV) { exists = true; break; }
Okay, with this change the 06/12 patch makes sense. As pointed out there small reordering is required. ACK Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 107 +++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 30da953446..36278ebc1c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13335,10 +13335,6 @@ static const vshCmdInfo info_event[] = { static const vshCmdOptDef opts_event[] = { VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), 0), - {.name = "event", - .type = VSH_OT_STRING, - .help = N_("which event type to wait for") - }, {.name = "all", .type = VSH_OT_BOOL, .help = N_("wait for all events instead of just one type") @@ -13359,6 +13355,10 @@ static const vshCmdOptDef opts_event[] = { .type = VSH_OT_BOOL, .help = N_("show timestamp for each printed event") }, + {.name = "event", + .type = VSH_OT_ARGV, + .help = N_("which event type to wait for") + }, {.name = NULL} }; @@ -13368,58 +13368,64 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; int timeout = 0; - virshDomEventData *data = NULL; + virshDomEventData *dataList = NULL; size_t i; - const char *eventName = NULL; - int event = -1; + int *eventIdxList = NULL; + size_t nevents = 0; + int eventid = -1; bool all = vshCommandOptBool(cmd, "all"); bool loop = vshCommandOptBool(cmd, "loop"); bool timestamp = vshCommandOptBool(cmd, "timestamp"); + bool event = vshCommandOptBool(cmd, "event"); int count = 0; + const vshCmdOpt *opt = NULL; virshControlPtr priv = ctl->privData; if (vshCommandOptBool(cmd, "list")) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - vshPrint(ctl, "%s\n", vshEventCallbacks[event].name); + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + vshPrint(ctl, "%s\n", vshEventCallbacks[eventid].name); return true; } - if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0) - return false; - if (eventName) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - if (STREQ(eventName, vshEventCallbacks[event].name)) - break; - if (event == VIR_DOMAIN_EVENT_ID_LAST) { - vshError(ctl, _("unknown event type %s"), eventName); - return false; + if (event) { + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { + if (opt->data) { + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + if (STREQ(opt->data, vshEventCallbacks[eventid].name)) + break; + if (eventid == VIR_DOMAIN_EVENT_ID_LAST) { + vshError(ctl, _("unknown event type %s"), opt->data); + goto cleanup; + } + size_t n = nevents; + virshDomEventData data; + data.ctl = ctl; + data.loop = loop; + data.count = &count; + data.timestamp = timestamp; + data.cb = &vshEventCallbacks[eventid]; + data.id = -1; + if (VIR_APPEND_ELEMENT(dataList, nevents, data) < 0) + goto cleanup; + if (VIR_APPEND_ELEMENT(eventIdxList, n, eventid) < 0) + goto cleanup; + } } - } else if (!all) { - vshError(ctl, "%s", - _("one of --list, --all, or --event <type> is required")); - return false; - } - - if (all) { - if (VIR_ALLOC_N(data, VIR_DOMAIN_EVENT_ID_LAST) < 0) + } else if (all) { + if (VIR_ALLOC_N(dataList, VIR_DOMAIN_EVENT_ID_LAST) < 0) goto cleanup; for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { - data[i].ctl = ctl; - data[i].loop = loop; - data[i].count = &count; - data[i].timestamp = timestamp; - data[i].cb = &vshEventCallbacks[i]; - data[i].id = -1; + dataList[i].ctl = ctl; + dataList[i].loop = loop; + dataList[i].count = &count; + dataList[i].timestamp = timestamp; + dataList[i].cb = &vshEventCallbacks[i]; + dataList[i].id = -1; } } else { - if (VIR_ALLOC_N(data, 1) < 0) - goto cleanup; - data[0].ctl = ctl; - data[0].loop = vshCommandOptBool(cmd, "loop"); - data[0].count = &count; - data[0].timestamp = timestamp; - data[0].cb = &vshEventCallbacks[event]; - data[0].id = -1; + vshError(ctl, "%s", + _("one of --list, --all, or --event <type> is required")); + return false; } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) goto cleanup; @@ -13431,11 +13437,11 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) if (vshEventStart(ctl, timeout) < 0) goto cleanup; - for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if ((data[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, - all ? i : event, - data[i].cb->cb, - &data[i], + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if ((dataList[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, + event ? eventIdxList[i] : i, + dataList[i].cb->cb, + &dataList[i], NULL)) < 0) { /* When registering for all events: if the first * registration succeeds, silently ignore failures on all @@ -13465,14 +13471,15 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) cleanup: vshEventCleanup(ctl); - if (data) { - for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if (data[i].id >= 0 && - virConnectDomainEventDeregisterAny(priv->conn, data[i].id) < 0) + if (dataList) { + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if (dataList[i].id >= 0 && + virConnectDomainEventDeregisterAny(priv->conn, dataList[i].id) < 0) ret = false; } - VIR_FREE(data); + VIR_FREE(dataList); } + VIR_FREE(eventIdxList); virshDomainFree(dom); return ret; } -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 107 +++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 50 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 30da953446..36278ebc1c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13335,10 +13335,6 @@ static const vshCmdInfo info_event[] = { static const vshCmdOptDef opts_event[] = { VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), 0), - {.name = "event", - .type = VSH_OT_STRING, - .help = N_("which event type to wait for") - }, {.name = "all", .type = VSH_OT_BOOL, .help = N_("wait for all events instead of just one type") @@ -13359,6 +13355,10 @@ static const vshCmdOptDef opts_event[] = { .type = VSH_OT_BOOL, .help = N_("show timestamp for each printed event") }, + {.name = "event", + .type = VSH_OT_ARGV, + .help = N_("which event type to wait for") + }, {.name = NULL} };
I'm not sure we can do this. Consider somebody has the following line in their script: virsh event domain reboot (which is supposed to wait for reboot event on domain "domain"). However, after your change virsh fails. This would introduce a regression. Not mentioning the fact that documentation needs update too ;-)
@@ -13368,58 +13368,64 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; int timeout = 0; - virshDomEventData *data = NULL; + virshDomEventData *dataList = NULL; size_t i; - const char *eventName = NULL; - int event = -1; + int *eventIdxList = NULL; + size_t nevents = 0; + int eventid = -1; bool all = vshCommandOptBool(cmd, "all"); bool loop = vshCommandOptBool(cmd, "loop"); bool timestamp = vshCommandOptBool(cmd, "timestamp"); + bool event = vshCommandOptBool(cmd, "event"); int count = 0; + const vshCmdOpt *opt = NULL; virshControlPtr priv = ctl->privData;
if (vshCommandOptBool(cmd, "list")) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - vshPrint(ctl, "%s\n", vshEventCallbacks[event].name); + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + vshPrint(ctl, "%s\n", vshEventCallbacks[eventid].name); return true; }
- if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0) - return false; - if (eventName) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - if (STREQ(eventName, vshEventCallbacks[event].name)) - break; - if (event == VIR_DOMAIN_EVENT_ID_LAST) { - vshError(ctl, _("unknown event type %s"), eventName); - return false; + if (event) { + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { + if (opt->data) { + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + if (STREQ(opt->data, vshEventCallbacks[eventid].name)) + break; + if (eventid == VIR_DOMAIN_EVENT_ID_LAST) { + vshError(ctl, _("unknown event type %s"), opt->data); + goto cleanup; + } + size_t n = nevents; + virshDomEventData data; + data.ctl = ctl; + data.loop = loop; + data.count = &count; + data.timestamp = timestamp; + data.cb = &vshEventCallbacks[eventid]; + data.id = -1; + if (VIR_APPEND_ELEMENT(dataList, nevents, data) < 0) + goto cleanup; + if (VIR_APPEND_ELEMENT(eventIdxList, n, eventid) < 0) + goto cleanup; + } } - } else if (!all) { - vshError(ctl, "%s", - _("one of --list, --all, or --event <type> is required")); - return false; - } - - if (all) { - if (VIR_ALLOC_N(data, VIR_DOMAIN_EVENT_ID_LAST) < 0) + } else if (all) { + if (VIR_ALLOC_N(dataList, VIR_DOMAIN_EVENT_ID_LAST) < 0) goto cleanup; for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { - data[i].ctl = ctl; - data[i].loop = loop; - data[i].count = &count; - data[i].timestamp = timestamp; - data[i].cb = &vshEventCallbacks[i]; - data[i].id = -1; + dataList[i].ctl = ctl; + dataList[i].loop = loop; + dataList[i].count = &count; + dataList[i].timestamp = timestamp; + dataList[i].cb = &vshEventCallbacks[i]; + dataList[i].id = -1;
How about doing simple: if (event) { parse events into @eventIdxList } else if (all) { VIR_ALLOC_N(eventIdxList, VIR_DOMAIN_EVENT_ID_LAST); for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) eventIdxList[i] = i; } The idea is that eventIdxList will be populated in both cases. It simplifies the code a bit because then we don't have to special case on all the places. The @dataList can be constructed outside of those if bodies then.
} } else { - if (VIR_ALLOC_N(data, 1) < 0) - goto cleanup; - data[0].ctl = ctl; - data[0].loop = vshCommandOptBool(cmd, "loop"); - data[0].count = &count; - data[0].timestamp = timestamp; - data[0].cb = &vshEventCallbacks[event]; - data[0].id = -1; + vshError(ctl, "%s", + _("one of --list, --all, or --event <type> is required")); + return false; } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) goto cleanup; @@ -13431,11 +13437,11 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) if (vshEventStart(ctl, timeout) < 0) goto cleanup;
- for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if ((data[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, - all ? i : event, - data[i].cb->cb, - &data[i], + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if ((dataList[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, + event ? eventIdxList[i] : i, + dataList[i].cb->cb, + &dataList[i], NULL)) < 0) { /* When registering for all events: if the first * registration succeeds, silently ignore failures on all @@ -13465,14 +13471,15 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
cleanup: vshEventCleanup(ctl); - if (data) { - for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if (data[i].id >= 0 && - virConnectDomainEventDeregisterAny(priv->conn, data[i].id) < 0) + if (dataList) { + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if (dataList[i].id >= 0 && + virConnectDomainEventDeregisterAny(priv->conn, dataList[i].id) < 0) ret = false; } - VIR_FREE(data); + VIR_FREE(dataList); } + VIR_FREE(eventIdxList); virshDomainFree(dom); return ret; }
Again, problem is that we cannot do the argument swap like you did. But without it the rest of the patch is needless. Michal
On 05/10/2018 05:17 PM, Michal Privoznik wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 107 +++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 50 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 30da953446..36278ebc1c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13335,10 +13335,6 @@ static const vshCmdInfo info_event[] = { static const vshCmdOptDef opts_event[] = { VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), 0), - {.name = "event", - .type = VSH_OT_STRING, - .help = N_("which event type to wait for") - }, {.name = "all", .type = VSH_OT_BOOL, .help = N_("wait for all events instead of just one type") @@ -13359,6 +13355,10 @@ static const vshCmdOptDef opts_event[] = { .type = VSH_OT_BOOL, .help = N_("show timestamp for each printed event") }, + {.name = "event", + .type = VSH_OT_ARGV, + .help = N_("which event type to wait for") + }, {.name = NULL} }; I'm not sure we can do this. Consider somebody has the following line in
On 05/08/2018 04:20 PM, Lin Ma wrote: their script:
virsh event domain reboot
(which is supposed to wait for reboot event on domain "domain").
However, after your change virsh fails. This would introduce a regression. Not mentioning the fact that documentation needs update too ;-)
@@ -13368,58 +13368,64 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; int timeout = 0; - virshDomEventData *data = NULL; + virshDomEventData *dataList = NULL; size_t i; - const char *eventName = NULL; - int event = -1; + int *eventIdxList = NULL; + size_t nevents = 0; + int eventid = -1; bool all = vshCommandOptBool(cmd, "all"); bool loop = vshCommandOptBool(cmd, "loop"); bool timestamp = vshCommandOptBool(cmd, "timestamp"); + bool event = vshCommandOptBool(cmd, "event"); int count = 0; + const vshCmdOpt *opt = NULL; virshControlPtr priv = ctl->privData;
if (vshCommandOptBool(cmd, "list")) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - vshPrint(ctl, "%s\n", vshEventCallbacks[event].name); + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + vshPrint(ctl, "%s\n", vshEventCallbacks[eventid].name); return true; }
- if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0) - return false; - if (eventName) { - for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) - if (STREQ(eventName, vshEventCallbacks[event].name)) - break; - if (event == VIR_DOMAIN_EVENT_ID_LAST) { - vshError(ctl, _("unknown event type %s"), eventName); - return false; + if (event) { + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { + if (opt->data) { + for (eventid = 0; eventid < VIR_DOMAIN_EVENT_ID_LAST; eventid++) + if (STREQ(opt->data, vshEventCallbacks[eventid].name)) + break; + if (eventid == VIR_DOMAIN_EVENT_ID_LAST) { + vshError(ctl, _("unknown event type %s"), opt->data); + goto cleanup; + } + size_t n = nevents; + virshDomEventData data; + data.ctl = ctl; + data.loop = loop; + data.count = &count; + data.timestamp = timestamp; + data.cb = &vshEventCallbacks[eventid]; + data.id = -1; + if (VIR_APPEND_ELEMENT(dataList, nevents, data) < 0) + goto cleanup; + if (VIR_APPEND_ELEMENT(eventIdxList, n, eventid) < 0) + goto cleanup; + } } - } else if (!all) { - vshError(ctl, "%s", - _("one of --list, --all, or --event <type> is required")); - return false; - } - - if (all) { - if (VIR_ALLOC_N(data, VIR_DOMAIN_EVENT_ID_LAST) < 0) + } else if (all) { + if (VIR_ALLOC_N(dataList, VIR_DOMAIN_EVENT_ID_LAST) < 0) goto cleanup; for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { - data[i].ctl = ctl; - data[i].loop = loop; - data[i].count = &count; - data[i].timestamp = timestamp; - data[i].cb = &vshEventCallbacks[i]; - data[i].id = -1; + dataList[i].ctl = ctl; + dataList[i].loop = loop; + dataList[i].count = &count; + dataList[i].timestamp = timestamp; + dataList[i].cb = &vshEventCallbacks[i]; + dataList[i].id = -1;
How about doing simple:
if (event) { parse events into @eventIdxList } else if (all) { VIR_ALLOC_N(eventIdxList, VIR_DOMAIN_EVENT_ID_LAST); for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) eventIdxList[i] = i; }
The idea is that eventIdxList will be populated in both cases. It simplifies the code a bit because then we don't have to special case on all the places. The @dataList can be constructed outside of those if bodies then.
} } else { - if (VIR_ALLOC_N(data, 1) < 0) - goto cleanup; - data[0].ctl = ctl; - data[0].loop = vshCommandOptBool(cmd, "loop"); - data[0].count = &count; - data[0].timestamp = timestamp; - data[0].cb = &vshEventCallbacks[event]; - data[0].id = -1; + vshError(ctl, "%s", + _("one of --list, --all, or --event <type> is required")); + return false; } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) goto cleanup; @@ -13431,11 +13437,11 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) if (vshEventStart(ctl, timeout) < 0) goto cleanup;
- for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if ((data[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, - all ? i : event, - data[i].cb->cb, - &data[i], + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if ((dataList[i].id = virConnectDomainEventRegisterAny(priv->conn, dom, + event ? eventIdxList[i] : i, + dataList[i].cb->cb, + &dataList[i], NULL)) < 0) { /* When registering for all events: if the first * registration succeeds, silently ignore failures on all @@ -13465,14 +13471,15 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
cleanup: vshEventCleanup(ctl); - if (data) { - for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) { - if (data[i].id >= 0 && - virConnectDomainEventDeregisterAny(priv->conn, data[i].id) < 0) + if (dataList) { + for (i = 0; i < (event ? nevents : VIR_DOMAIN_EVENT_ID_LAST); i++) { + if (dataList[i].id >= 0 && + virConnectDomainEventDeregisterAny(priv->conn, dataList[i].id) < 0) ret = false; } - VIR_FREE(data); + VIR_FREE(dataList); } + VIR_FREE(eventIdxList); virshDomainFree(dom); return ret; }
Again, problem is that we cannot do the argument swap like you did. But without it the rest of the patch is needless. Hmm, If we can't change the type to 'VSH_OT_ARGV', then this patch is needless. OK, Let's forget this patch for the time being.
Thanks, Lin
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-util.h | 3 +++ 2 files changed, 63 insertions(+) diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64b..686f9aef98 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -285,3 +285,63 @@ virshDomainGetXML(vshControl *ctl, return ret; } + + +const char * +virshDomainEventGetName(int event) +{ + switch ((int)event) { + case VIR_DOMAIN_EVENT_ID_LIFECYCLE: + return "lifecycle"; + case VIR_DOMAIN_EVENT_ID_REBOOT: + return "reboot"; + case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: + return "rtc-change"; + case VIR_DOMAIN_EVENT_ID_WATCHDOG: + return "watchdog"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR: + return "io-error"; + case VIR_DOMAIN_EVENT_ID_GRAPHICS: + return "graphics"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: + return "io-error-reason"; + case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: + return "control-error"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + return "block-job"; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + return "disk-change"; + case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: + return "tray-change"; + case VIR_DOMAIN_EVENT_ID_PMWAKEUP: + return "pm-wakeup"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND: + return "pm-suspend"; + case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: + return "balloon-change"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: + return "pm-suspend-disk"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + return "device-removed"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: + return "block-job-2"; + case VIR_DOMAIN_EVENT_ID_TUNABLE: + return "tunable"; + case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: + return "agent-lifecycle"; + case VIR_DOMAIN_EVENT_ID_DEVICE_ADDED: + return "device-added"; + case VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION: + return "migration-iteration"; + case VIR_DOMAIN_EVENT_ID_JOB_COMPLETED: + return "job-completed"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + return "device-removal-failed"; + case VIR_DOMAIN_EVENT_ID_METADATA_CHANGE: + return "metadata-change"; + case VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD: + return "block-threshold"; + default: + return NULL; + } +} diff --git a/tools/virsh-util.h b/tools/virsh-util.h index 9a0af3513d..02c4ace6bf 100644 --- a/tools/virsh-util.h +++ b/tools/virsh-util.h @@ -104,4 +104,7 @@ virshDomainGetXML(vshControl *ctl, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; +const char * +virshDomainEventGetName(int event); + #endif /* VIRSH_UTIL_H */ -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-util.h | 3 +++ 2 files changed, 63 insertions(+)
diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64b..686f9aef98 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -285,3 +285,63 @@ virshDomainGetXML(vshControl *ctl,
return ret; } + + +const char * +virshDomainEventGetName(int event) +{ + switch ((int)event) { + case VIR_DOMAIN_EVENT_ID_LIFECYCLE: + return "lifecycle"; + case VIR_DOMAIN_EVENT_ID_REBOOT: + return "reboot"; + case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: + return "rtc-change"; + case VIR_DOMAIN_EVENT_ID_WATCHDOG: + return "watchdog"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR: + return "io-error"; + case VIR_DOMAIN_EVENT_ID_GRAPHICS: + return "graphics"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: + return "io-error-reason"; + case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: + return "control-error"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + return "block-job"; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + return "disk-change"; + case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: + return "tray-change"; + case VIR_DOMAIN_EVENT_ID_PMWAKEUP: + return "pm-wakeup"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND: + return "pm-suspend"; + case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: + return "balloon-change"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: + return "pm-suspend-disk"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + return "device-removed"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: + return "block-job-2"; + case VIR_DOMAIN_EVENT_ID_TUNABLE: + return "tunable"; + case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: + return "agent-lifecycle"; + case VIR_DOMAIN_EVENT_ID_DEVICE_ADDED: + return "device-added"; + case VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION: + return "migration-iteration"; + case VIR_DOMAIN_EVENT_ID_JOB_COMPLETED: + return "job-completed"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + return "device-removal-failed"; + case VIR_DOMAIN_EVENT_ID_METADATA_CHANGE: + return "metadata-change"; + case VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD: + return "block-threshold"; + default: + return NULL; + }
Or simply: const char * virshDomainEventGetName(int event) { if (event < 0 || event >= VIR_DOMAIN_EVENT_ID_LAST) return NULL; return vshEventCallbacks[event].name; } I really want to avoid having two places defining strings (one for command parse code, the other for completions). Michal
On 05/10/2018 05:17 PM, Michal Privoznik wrote:
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-util.h | 3 +++ 2 files changed, 63 insertions(+)
diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64b..686f9aef98 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -285,3 +285,63 @@ virshDomainGetXML(vshControl *ctl,
return ret; } + + +const char * +virshDomainEventGetName(int event) +{ + switch ((int)event) { + case VIR_DOMAIN_EVENT_ID_LIFECYCLE: + return "lifecycle"; + case VIR_DOMAIN_EVENT_ID_REBOOT: + return "reboot"; + case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: + return "rtc-change"; + case VIR_DOMAIN_EVENT_ID_WATCHDOG: + return "watchdog"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR: + return "io-error"; + case VIR_DOMAIN_EVENT_ID_GRAPHICS: + return "graphics"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: + return "io-error-reason"; + case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: + return "control-error"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + return "block-job"; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + return "disk-change"; + case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: + return "tray-change"; + case VIR_DOMAIN_EVENT_ID_PMWAKEUP: + return "pm-wakeup"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND: + return "pm-suspend"; + case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: + return "balloon-change"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: + return "pm-suspend-disk"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + return "device-removed"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: + return "block-job-2"; + case VIR_DOMAIN_EVENT_ID_TUNABLE: + return "tunable"; + case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: + return "agent-lifecycle"; + case VIR_DOMAIN_EVENT_ID_DEVICE_ADDED: + return "device-added"; + case VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION: + return "migration-iteration"; + case VIR_DOMAIN_EVENT_ID_JOB_COMPLETED: + return "job-completed"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + return "device-removal-failed"; + case VIR_DOMAIN_EVENT_ID_METADATA_CHANGE: + return "metadata-change"; + case VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD: + return "block-threshold"; + default: + return NULL; + }
Or simply:
const char * virshDomainEventGetName(int event) { if (event < 0 || event >= VIR_DOMAIN_EVENT_ID_LAST) return NULL;
return vshEventCallbacks[event].name; }
I really want to avoid having two places defining strings (one for command parse code, the other for completions). For using vshEventCallbacks in tools/virsh-util.c, It seems that we need to move lots of code which related to vshEventCallbacks to virsh-util.h or somewhere else. Say those event callback functions and corresponding structures and macros.
I'm not sure what is the reasonable way to make it happen, May I have your idea? (btw, there are other 'vshEventCallbacks' array which defining in virsh-secret.c, virsh-pool.c and virsh-nodedev.c.we only move the vshEventCallbacks and related data structures from virsh-domain.c?) Thanks, Lin
On 05/11/2018 10:22 AM, Lin Ma wrote:
On 05/10/2018 05:17 PM, Michal Privoznik wrote:
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-util.h | 3 +++ 2 files changed, 63 insertions(+)
diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64b..686f9aef98 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -285,3 +285,63 @@ virshDomainGetXML(vshControl *ctl, return ret; } + + +const char * +virshDomainEventGetName(int event) +{ + switch ((int)event) { + case VIR_DOMAIN_EVENT_ID_LIFECYCLE: + return "lifecycle"; + case VIR_DOMAIN_EVENT_ID_REBOOT: + return "reboot"; + case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: + return "rtc-change"; + case VIR_DOMAIN_EVENT_ID_WATCHDOG: + return "watchdog"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR: + return "io-error"; + case VIR_DOMAIN_EVENT_ID_GRAPHICS: + return "graphics"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: + return "io-error-reason"; + case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: + return "control-error"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + return "block-job"; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + return "disk-change"; + case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: + return "tray-change"; + case VIR_DOMAIN_EVENT_ID_PMWAKEUP: + return "pm-wakeup"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND: + return "pm-suspend"; + case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: + return "balloon-change"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: + return "pm-suspend-disk"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + return "device-removed"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: + return "block-job-2"; + case VIR_DOMAIN_EVENT_ID_TUNABLE: + return "tunable"; + case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: + return "agent-lifecycle"; + case VIR_DOMAIN_EVENT_ID_DEVICE_ADDED: + return "device-added"; + case VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION: + return "migration-iteration"; + case VIR_DOMAIN_EVENT_ID_JOB_COMPLETED: + return "job-completed"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + return "device-removal-failed"; + case VIR_DOMAIN_EVENT_ID_METADATA_CHANGE: + return "metadata-change"; + case VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD: + return "block-threshold"; + default: + return NULL; + }
Or simply:
const char * virshDomainEventGetName(int event) { if (event < 0 || event >= VIR_DOMAIN_EVENT_ID_LAST) return NULL;
return vshEventCallbacks[event].name; }
I really want to avoid having two places defining strings (one for command parse code, the other for completions). For using vshEventCallbacks in tools/virsh-util.c, It seems that we need to move lots of code which related to vshEventCallbacks to virsh-util.h or somewhere else. Say those event callback functions and corresponding structures and macros.
I'm not sure what is the reasonable way to make it happen, May I have your idea?
Well, the virshDomainEventGetName() function doesn't necessarily have to live in virsh-utils.c. And now that I'm looking into other sources, maybe this function can be renamed to virshDomainEventToString()? To match the other functions like virshSecretEventToString() for instance.
(btw, there are other 'vshEventCallbacks' array which defining in virsh-secret.c, virsh-pool.c and virsh-nodedev.c.we only move the vshEventCallbacks and related data structures from virsh-domain.c?)
We can have virshSecretEventToString(), virshPoolEventToString(), ... exposed and used in completers. Michal
On 05/11/2018 05:03 PM, Michal Privoznik wrote:
On 05/11/2018 10:22 AM, Lin Ma wrote:
On 05/10/2018 05:17 PM, Michal Privoznik wrote:
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-util.h | 3 +++ 2 files changed, 63 insertions(+)
diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64b..686f9aef98 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -285,3 +285,63 @@ virshDomainGetXML(vshControl *ctl, return ret; } + + +const char * +virshDomainEventGetName(int event) +{ + switch ((int)event) { + case VIR_DOMAIN_EVENT_ID_LIFECYCLE: + return "lifecycle"; + case VIR_DOMAIN_EVENT_ID_REBOOT: + return "reboot"; + case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: + return "rtc-change"; + case VIR_DOMAIN_EVENT_ID_WATCHDOG: + return "watchdog"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR: + return "io-error"; + case VIR_DOMAIN_EVENT_ID_GRAPHICS: + return "graphics"; + case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: + return "io-error-reason"; + case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: + return "control-error"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: + return "block-job"; + case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: + return "disk-change"; + case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: + return "tray-change"; + case VIR_DOMAIN_EVENT_ID_PMWAKEUP: + return "pm-wakeup"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND: + return "pm-suspend"; + case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: + return "balloon-change"; + case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: + return "pm-suspend-disk"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + return "device-removed"; + case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: + return "block-job-2"; + case VIR_DOMAIN_EVENT_ID_TUNABLE: + return "tunable"; + case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: + return "agent-lifecycle"; + case VIR_DOMAIN_EVENT_ID_DEVICE_ADDED: + return "device-added"; + case VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION: + return "migration-iteration"; + case VIR_DOMAIN_EVENT_ID_JOB_COMPLETED: + return "job-completed"; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + return "device-removal-failed"; + case VIR_DOMAIN_EVENT_ID_METADATA_CHANGE: + return "metadata-change"; + case VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD: + return "block-threshold"; + default: + return NULL; + } Or simply:
const char * virshDomainEventGetName(int event) { if (event < 0 || event >= VIR_DOMAIN_EVENT_ID_LAST) return NULL;
return vshEventCallbacks[event].name; }
I really want to avoid having two places defining strings (one for command parse code, the other for completions). For using vshEventCallbacks in tools/virsh-util.c, It seems that we need to move lots of code which related to vshEventCallbacks to virsh-util.h or somewhere else. Say those event callback functions and corresponding structures and macros.
I'm not sure what is the reasonable way to make it happen, May I have your idea?
Well, the virshDomainEventGetName() function doesn't necessarily have to live in virsh-utils.c. And now that I'm looking into other sources, maybe this function can be renamed to virshDomainEventToString()? To match the other functions like virshSecretEventToString() for instance.
(btw, there are other 'vshEventCallbacks' array which defining in virsh-secret.c, virsh-pool.c and virsh-nodedev.c.we only move the vshEventCallbacks and related data structures from virsh-domain.c?)
We can have virshSecretEventToString(), virshPoolEventToString(), ... exposed and used in completers. The function names about *EventToString are already occupied, So I'd like to use the original name virshDomainEventGetName for the time being.
Because the function virshDomainEventGetName only be used for virsh, I think I don't need to export it. I prefer to put the implmentation of function virshDomainEventGetName into virsh-domain.c and declare it in virsh-domain.h, Then virsh-completer.c includes virsh-domain.h for using virshDomainEventGetName in next patch, What do you think? Thanks, Lin
On 05/15/2018 11:32 AM, Lin Ma wrote:
(btw, there are other 'vshEventCallbacks' array which defining in virsh-secret.c, virsh-pool.c and virsh-nodedev.c.we only move the vshEventCallbacks and related data structures from virsh-domain.c?) We can have virshSecretEventToString(), virshPoolEventToString(), ... exposed and used in completers. The function names about *EventToString are already occupied,
Oh, we can't just export them. Not because of they already exist but because they don't really do what we expect them to do. At least virshSecretEventToString() doesn't. Anyway, I'm including a patch that does what I'm trying to say. Also, virshSecretEventToString() should be renamed to virshSecretLifecycleEventToString(). Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer.c | 37 +++++++++++++++++++++++++++++++++++++ tools/virsh-completer.h | 3 +++ tools/virsh-domain.c | 1 + 3 files changed, 41 insertions(+) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index e3b8234b41..ddf7b17caf 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -522,3 +522,40 @@ virshSnapshotNameCompleter(vshControl *ctl, virshDomainFree(dom); return NULL; } + + +char ** +virshEventNameCompleter(vshControl *ctl, + const vshCmd *cmd ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + size_t i = 0; + char **ret = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (VIR_ALLOC_N(ret, VIR_DOMAIN_EVENT_ID_LAST + 1) < 0) + goto error; + + for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { + const char *name = virshDomainEventGetName(i); + + if (name == NULL) + goto error; + + if (VIR_STRDUP(ret[i], name) < 0) + goto error; + } + + return ret; + + error: + for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) + VIR_FREE(ret[i]); + VIR_FREE(ret); + return NULL; +} diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h index fa443d3ad7..27d78dc7ac 100644 --- a/tools/virsh-completer.h +++ b/tools/virsh-completer.h @@ -69,5 +69,8 @@ char ** virshSecretUUIDCompleter(vshControl *ctl, char ** virshSnapshotNameCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); +char ** virshEventNameCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); #endif diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 36278ebc1c..0d58c0ff1a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13357,6 +13357,7 @@ static const vshCmdOptDef opts_event[] = { }, {.name = "event", .type = VSH_OT_ARGV, + .completer = virshEventNameCompleter, .help = N_("which event type to wait for") }, {.name = NULL} -- 2.15.1
On 05/08/2018 04:20 PM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer.c | 37 +++++++++++++++++++++++++++++++++++++ tools/virsh-completer.h | 3 +++ tools/virsh-domain.c | 1 + 3 files changed, 41 insertions(+)
This one looks okay, but it relies on patch 11/12 which wasn't ACKed. Michal
On 05/08/2018 04:20 PM, Lin Ma wrote:
This patch series is about some fixes and improvement for virsh completion in non interactive mode.
I've ACKed patches 01-09/12 (after fixing some of them), and pushed them. Patch 10/12 is no go I fear, patch 11/12 needs some rework and finally patch 12/12 is okay, but relies on previous patch, so I can't push it. Michal
participants (2)
-
Lin Ma -
Michal Privoznik