[libvirt] [PATCHv3 0/6] virsh: More intelligent auto-completion
Hi, this patch series is a prototype for my GSoC project (Michal Privoznik is my mentor). I'm working on virsh auto-completion, trying to make it more "intelligent". At this stage, prototype is capable of command and option completion. Three completer functions are currently implemented so you can test it. If it turns out that this prototype is good enough, I will implement more completer functions. --- v3: * vshReconnect() is now called only when we reach the point that completion is being attempted on commands that needs connection * moved all .completer intializations into the 4/6, 5/6 and 6/6 v2: https://www.redhat.com/archives/libvir-list/2013-August/msg00992.html v1: https://www.redhat.com/archives/libvir-list/2013-August/msg00371.html Tomas Meszaros (6): virsh: C99 style for info_domfstrim and opts_lxc_enter_namespace virsh: Add vshCmdCompleter and vshOptCompleter virsh: Improve readline generators and readline completion virsh: Add vshDomainCompleter virsh: Add vshSuspendTargetCompleter virsh: Add vshRebootShutdownModeCompleter .gnulib | 2 +- tools/virsh-domain-monitor.c | 32 ++- tools/virsh-domain.c | 248 +++++++++++++++++----- tools/virsh-snapshot.c | 45 +++- tools/virsh.c | 480 +++++++++++++++++++++++++++++++++++++++++-- tools/virsh.h | 11 + 6 files changed, 725 insertions(+), 93 deletions(-) -- 1.8.3.1
Change info_domfstrim and opts_lxc_enter_namespace initialization style to C99. --- tools/virsh-domain.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b29f934..5d4913d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7903,10 +7903,21 @@ static const vshCmdInfo info_lxc_enter_namespace[] = { }; static const vshCmdOptDef opts_lxc_enter_namespace[] = { - {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"noseclabel", VSH_OT_BOOL, 0, N_("Do not change process security label")}, - {"cmd", VSH_OT_ARGV, VSH_OFLAG_REQ, N_("namespace")}, - {NULL, 0, 0, NULL} + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "noseclabel", + .type = VSH_OT_BOOL, + .help = N_("Do not change process security label") + }, + {.name = "cmd", + .type = VSH_OT_ARGV, + .flags = VSH_OFLAG_REQ, + .help = N_("namespace") + }, + {.name = NULL} }; static bool @@ -10290,7 +10301,7 @@ static const vshCmdOptDef opts_domfstrim[] = { .type = VSH_OT_DATA, .help = N_("which mount point to trim") }, - {NULL, 0, 0, NULL} + {.name = NULL} }; static bool cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
Change info_domfstrim and opts_lxc_enter_namespace initialization style to C99. --- tools/virsh-domain.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
ACK and pushed. Congrats on your first libvirt patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
completer and completer_flags added to the _vshCmdDef and _vshCmdOptDef structures so it will be possible for completion generators to conveniently call completer functions with desired flags. --- tools/virsh.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/virsh.h b/tools/virsh.h index 466ca2d..064acde 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -147,6 +147,9 @@ typedef struct _vshCmdOptDef vshCmdOptDef; typedef struct _vshControl vshControl; typedef struct _vshCtrlData vshCtrlData; +typedef char **(*vshCmdCompleter)(unsigned int flags); +typedef char **(*vshOptCompleter)(unsigned int flags); + /* * vshCmdInfo -- name/value pair for information about command * @@ -168,6 +171,8 @@ struct _vshCmdOptDef { unsigned int flags; /* flags */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ + vshOptCompleter completer; /* option completer */ + unsigned int completer_flags; /* option completer flags */ }; /* @@ -199,6 +204,8 @@ struct _vshCmdDef { const vshCmdOptDef *opts; /* definition of command options */ const vshCmdInfo *info; /* details about command */ unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */ + vshCmdCompleter completer; /* command completer */ + unsigned int completer_flags; /* command completer flags */ }; /* -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
completer and completer_flags added to the _vshCmdDef and _vshCmdOptDef structures so it will be possible for completion generators to conveniently call completer functions with desired flags. --- tools/virsh.h | 7 +++++++ 1 file changed, 7 insertions(+)
In isolation, this patch looks reasonable, but I want to see how it gets used before acking it.
diff --git a/tools/virsh.h b/tools/virsh.h index 466ca2d..064acde 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -147,6 +147,9 @@ typedef struct _vshCmdOptDef vshCmdOptDef; typedef struct _vshControl vshControl; typedef struct _vshCtrlData vshCtrlData;
+typedef char **(*vshCmdCompleter)(unsigned int flags); +typedef char **(*vshOptCompleter)(unsigned int flags);
Do we need two typedefs, or is (*vshCompleter) a sufficient reusable name?
+ /* * vshCmdInfo -- name/value pair for information about command * @@ -168,6 +171,8 @@ struct _vshCmdOptDef { unsigned int flags; /* flags */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ + vshOptCompleter completer; /* option completer */ + unsigned int completer_flags; /* option completer flags */
Per-option completions make sense. For example, 'virsh vol-key --pool <TAB>' wants to use a pool completer, while 'virsh vol-key --vol <TAB>' wants to use a volume completer; furthermore, 'virsh vol-key <TAB>' should be the combination of the option completer (showing --vol and --pool) AND the volume completer filtered to names not starting with '-' (since virsh has the semantics that arguments are positional, where the option '--vol' is implied if the argument that appears in that position does not resemble an option).
};
/* @@ -199,6 +204,8 @@ struct _vshCmdDef { const vshCmdOptDef *opts; /* definition of command options */ const vshCmdInfo *info; /* details about command */ unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */ + vshCmdCompleter completer; /* command completer */ + unsigned int completer_flags; /* command completer flags */ };
I'm not so sure about per-command completers, though. What can a command complete differently than per-option completers would already provide? Maybe this argues that you need more rationale in the commit message about how this will be used. Or maybe I'll figure it out by the time I get to the end of your series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 26/08/13 at 11:47am, Eric Blake wrote:
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
completer and completer_flags added to the _vshCmdDef and _vshCmdOptDef structures so it will be possible for completion generators to conveniently call completer functions with desired flags. --- tools/virsh.h | 7 +++++++ 1 file changed, 7 insertions(+)
In isolation, this patch looks reasonable, but I want to see how it gets used before acking it.
diff --git a/tools/virsh.h b/tools/virsh.h index 466ca2d..064acde 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -147,6 +147,9 @@ typedef struct _vshCmdOptDef vshCmdOptDef; typedef struct _vshControl vshControl; typedef struct _vshCtrlData vshCtrlData;
+typedef char **(*vshCmdCompleter)(unsigned int flags); +typedef char **(*vshOptCompleter)(unsigned int flags);
Do we need two typedefs, or is (*vshCompleter) a sufficient reusable name?
You are right, we don't need two typedefs. I will merge them into (*vshCompleter) as you are suggesting.
+ /* * vshCmdInfo -- name/value pair for information about command * @@ -168,6 +171,8 @@ struct _vshCmdOptDef { unsigned int flags; /* flags */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ + vshOptCompleter completer; /* option completer */ + unsigned int completer_flags; /* option completer flags */
Per-option completions make sense. For example, 'virsh vol-key --pool <TAB>' wants to use a pool completer, while 'virsh vol-key --vol <TAB>' wants to use a volume completer; furthermore, 'virsh vol-key <TAB>' should be the combination of the option completer (showing --vol and --pool) AND the volume completer filtered to names not starting with '-' (since virsh has the semantics that arguments are positional, where the option '--vol' is implied if the argument that appears in that position does not resemble an option).
So If I get it right, you are suggesting that it should work like this: virsh # vol-key <TAB> vol1 vol2 pool1 pool2 as you said, combination of --vol and --pool completers. I was initially thinking that completion should work like this: virsh # vol-key <TAB> vol1 vol2 It is completing <vol> first becasue <vol> is only mandatory argument for this command. Only if someone type: virsh # vol-key --pool <TAB> pool1 pool2 then call --pool completer.
};
/* @@ -199,6 +204,8 @@ struct _vshCmdDef { const vshCmdOptDef *opts; /* definition of command options */ const vshCmdInfo *info; /* details about command */ unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */ + vshCmdCompleter completer; /* command completer */ + unsigned int completer_flags; /* command completer flags */ };
I'm not so sure about per-command completers, though. What can a command complete differently than per-option completers would already provide? Maybe this argues that you need more rationale in the commit message about how this will be used. Or maybe I'll figure it out by the time I get to the end of your series.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- Tomas Meszaros
New completion generators responsible for advances command and command options completions. vshReadlineCommandCompletionGenerator - generator for advanced command completions. This function will call some vshCmdCompleter function (e.g. vshDomainCompleter), which will return relevant data used for autocompletion (e.g. domain names). vshReadlineOptionsCompletionGenerator - almost the same as the vshReadlineCommandCompletionGenerator, but this one completes cmd options. vshReadlineCompletion() has become much more complex because we now have more generator functions and therefore more states to choose from. --- v2 * vshMalloc is now used in vshDetermineCommandName * vshStrdup instead of vshMalloc + snprintf * joined if's in vshReadlineOptionsCompletionGenerator v3 * fixed typo * removed useless if's .gnulib | 2 +- tools/virsh.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 365 insertions(+), 23 deletions(-) diff --git a/.gnulib b/.gnulib index 0ba0877..644c404 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 0ba087759d2797c8f7d3c34bef6268ba3fd212cb +Subproject commit 644c40496cf7d5a705a73c9dd32b035fcecc2ab1 diff --git a/tools/virsh.c b/tools/virsh.c index 2ea44a6..5be8e7e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2515,6 +2515,25 @@ vshCloseLogFile(vshControl *ctl) * ----------------- */ +static const vshCmdDef * +vshDetermineCommandName(void) +{ + const vshCmdDef *cmd = NULL; + char *p; + char *cmdname; + + if (!(p = strchr(rl_line_buffer, ' '))) + return NULL; + + cmdname = vshMalloc(NULL, (p - rl_line_buffer) + 1); + memcpy(cmdname, rl_line_buffer, p - rl_line_buffer); + + cmd = vshCmddefSearch(cmdname); + VIR_FREE(cmdname); + + return cmd; +} + /* * Generator function for command completion. STATE lets us * know whether to start from scratch; without any state @@ -2562,25 +2581,14 @@ vshReadlineCommandGenerator(const char *text, int state) static char * vshReadlineOptionsGenerator(const char *text, int state) { - static int list_index, len; static const vshCmdDef *cmd = NULL; + static int list_index, len; const char *name; if (!state) { - /* determine command name */ - char *p; - char *cmdname; - - if (!(p = strchr(rl_line_buffer, ' '))) - return NULL; - - cmdname = vshCalloc(NULL, (p - rl_line_buffer) + 1, 1); - memcpy(cmdname, rl_line_buffer, p - rl_line_buffer); - - cmd = vshCmddefSearch(cmdname); + cmd = vshDetermineCommandName(); list_index = 0; len = strlen(text); - VIR_FREE(cmdname); } if (!cmd) @@ -2612,22 +2620,356 @@ vshReadlineOptionsGenerator(const char *text, int state) return NULL; } +/* + * Generator function for command completion, but unlike + * the vshReadlineCommandGenerator which completes command name, this function + * provides more advanced completion for commands by calling specific command + * completers (e.g. vshDomainCompleter). + */ +static char * +vshReadlineCommandCompletionGenerator(const char *text, int state) +{ + static const vshCmdDef *cmd = NULL; + static int list_index, len; + char **completed_names = NULL; + char *name; + + if (!state) { + cmd = vshDetermineCommandName(); + list_index = 0; + len = strlen(text); + } + + if (!cmd) + return NULL; + + if (!cmd->completer) + return NULL; + + completed_names = cmd->completer(cmd->completer_flags); + + if (!completed_names) + return NULL; + + while ((name = completed_names[list_index])) { + char *res; + list_index++; + + if (STRNEQLEN(name, text, len)) + /* Skip irrelevant names */ + continue; + + res = vshStrdup(NULL, name); + VIR_FREE(name); + return res; + } + VIR_FREE(completed_names); + + return NULL; +} + +/* + * Generator function for command option completion. Provides advances + * completion for command options. + */ +static char * +vshReadlineOptionsCompletionGenerator(const char *text ATTRIBUTE_UNUSED, + int state ATTRIBUTE_UNUSED) +{ + static const vshCmdDef *cmd = NULL; + static const vshCmdOptDef *opt = NULL; + static int list_index, len; + unsigned long int opt_index = 0; + size_t i; + char **completed_names = NULL; + char *name; + char *ptr = NULL; + + if (!state) { + cmd = vshDetermineCommandName(); + list_index = 0; + len = strlen(text); + } + + if (!cmd) + return NULL; + + if (!cmd->opts) + return NULL; + + for (i = 0; cmd->opts[i].name; i++) { + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name)) && + (opt_index < (ptr - rl_line_buffer))) { + opt_index = ptr - rl_line_buffer; + opt = &cmd->opts[i]; + } + } + + if (!opt) + return NULL; + + if (!opt->completer) + return NULL; + + completed_names = opt->completer(opt->completer_flags); + + if (!completed_names) + return NULL; + + while ((name = completed_names[list_index])) { + char *res; + list_index++; + + if (STRNEQLEN(name, text, len)) + /* Skip irrelevant names */ + continue; + + res = vshStrdup(NULL, name); + VIR_FREE(name); + return res; + } + VIR_FREE(completed_names); + + return NULL; +} + +/* + * Returns current valid command name present in the rl_line_buffer. + */ +static char * +vshCurrentCmd(void) +{ + const char *name; + const vshCmdGrp *grp; + const vshCmdDef *cmds; + size_t grp_list_index, cmd_list_index; + char *found_cmd = NULL; + char *rl_copy = NULL; + char *pch; + + grp_list_index = 0; + cmd_list_index = 0; + grp = cmdGroups; + + while (grp[grp_list_index].name) { + cmds = grp[grp_list_index].commands; + + if (cmds[cmd_list_index].name) { + while ((name = cmds[cmd_list_index].name)) { + cmd_list_index++; + + if (VIR_STRDUP(rl_copy, rl_line_buffer) < 0) + return NULL; + + char *saveptr; + pch = strtok_r(rl_copy, " ", &saveptr); + + while (pch != NULL) { + if (STREQ(pch, name)) + if (VIR_STRDUP(found_cmd, name) < 0) + goto cleanup; + pch = strtok_r(NULL, " ", &saveptr); + } + } + } else { + cmd_list_index = 0; + grp_list_index++; + } + } + + if (!found_cmd) + goto cleanup; + + return found_cmd; + +cleanup: + VIR_FREE(rl_copy); + return NULL; +} + +/* + * Returns current valid command option name present in the rl_line_buffer. + */ +static char * +vshCurrentOpt(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + const char *name; + unsigned long int opt_index = 0; + size_t cmd_opt_list_index; + char *found_opt = NULL; + char *ptr = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->opts) + return NULL; + + cmd_opt_list_index = 0; + + while ((name = cmd->opts[cmd_opt_list_index].name)) { + cmd_opt_list_index++; + + if ((ptr = strstr(rl_line_buffer, name))) { + if (opt_index < (ptr - rl_line_buffer)) { + opt_index = ptr - rl_line_buffer; + if (VIR_STRDUP(found_opt, name) < 0) + return NULL; + } + } + } + + return found_opt; +} + +/* + * Returns name of the command completion currently present in the rl_line_buffer. + */ +static char* +vshCurrentCmdCom(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + size_t i; + char **completed_names = NULL; + char *found_cmd_com = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->completer) + return NULL; + + completed_names = cmd->completer(cmd->completer_flags); + + if (!completed_names) + return NULL; + + for (i = 0; completed_names[i]; i++) { + if (strstr(rl_line_buffer, completed_names[i])) { + if (VIR_STRDUP(found_cmd_com, completed_names[i]) < 0) + return NULL; + } + } + + return found_cmd_com; +} + +/* + * Returns name of the option completion currently present in the rl_line_buffer. + */ +static char * +vshCurrentOptCom(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + const vshCmdOptDef *opt = NULL; + unsigned long int opt_index = 0; + size_t i; + char **completed_names = NULL; + char *found_opt_com = NULL; + char *ptr = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->opts) + return NULL; + + for (i = 0; cmd->opts[i].name; i++) { + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name))) { + if (opt_index < (ptr - rl_line_buffer)) { + opt_index = ptr - rl_line_buffer; + opt = &cmd->opts[i]; + } + } + } + + if (!opt) + return NULL; + + if (!opt->completer) + return NULL; + + completed_names = opt->completer(opt->completer_flags); + + if (!completed_names) + return NULL; + + for (i = 0; completed_names[i]; i++) { + if (strstr(rl_line_buffer, completed_names[i])) { + if (VIR_STRDUP(found_opt_com, completed_names[i]) < 0) + return NULL; + } + } + + return found_opt_com; +} + static char ** -vshReadlineCompletion(const char *text, int start, - int end ATTRIBUTE_UNUSED) +vshReadlineCompletion(const char *text, int start, int end ATTRIBUTE_UNUSED) { - char **matches = (char **) NULL; + const char *cmd = vshCurrentCmd(); + const char *cmd_com = vshCurrentCmdCom(cmd); + const char *opt = vshCurrentOpt(cmd); + const char *opt_com = vshCurrentOptCom(cmd); + char **matches = (char **)NULL; - if (start == 0) - /* command name generator */ + if (start == 0) { + /* Command name generator. */ matches = rl_completion_matches(text, vshReadlineCommandGenerator); - else - /* commands options */ - matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else { + /* Command completer, commands options and commands options completer + * generators. + */ + if (strstr(text, "-")) { + /* When user wants to see options. */ + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && !opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (cmd_com && !opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (cmd_com && opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && opt && opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (cmd_com && opt && opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } + } + + VIR_FREE(cmd); + VIR_FREE(cmd_com); + VIR_FREE(opt); + VIR_FREE(opt_com); + return matches; } - static int vshReadlineInit(vshControl *ctl) { -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
New completion generators responsible for advances command and command options completions.
Not sure if you meant s/advances/advanced/ ? It might help to add a link to http://cnswww.cns.cwru.edu/php/chet/readline/readline.html#SEC44, which is documentation on how readline custom completers work, as part of the commit message and/or in the code where you are installing custom completers.
.gnulib | 2 +- tools/virsh.c | 386 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 365 insertions(+), 23 deletions(-)
Oops, the change to .gnulib should NOT be here. You can fix it by rebasing this series (git rebase -i, then change 'pick' to 'edit' for this patch), and while on this patch, do 'git checkout HEAD^ .gnulib', then 'make' (which should automatically rerun ./autogen.sh and thus cover any fallout from the change to the submodule). This feels like a pretty big patch. It adds a number of new functions all at once; does it make sense to split this into multiple smaller patches, focusing on one thing at a time?
+++ b/tools/virsh.c @@ -2515,6 +2515,25 @@ vshCloseLogFile(vshControl *ctl) * ----------------- */
+static const vshCmdDef * +vshDetermineCommandName(void) +{ + const vshCmdDef *cmd = NULL; + char *p; + char *cmdname; + + if (!(p = strchr(rl_line_buffer, ' '))) + return NULL; + + cmdname = vshMalloc(NULL, (p - rl_line_buffer) + 1); + memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
Why not VIR_STRNDUP? Oh...
+ + cmd = vshCmddefSearch(cmdname); + VIR_FREE(cmdname); + + return cmd; +} + /* * Generator function for command completion. STATE lets us * know whether to start from scratch; without any state @@ -2562,25 +2581,14 @@ vshReadlineCommandGenerator(const char *text, int state) static char * vshReadlineOptionsGenerator(const char *text, int state) { - static int list_index, len; static const vshCmdDef *cmd = NULL; + static int list_index, len; const char *name;
if (!state) { - /* determine command name */ - char *p; - char *cmdname; - - if (!(p = strchr(rl_line_buffer, ' '))) - return NULL; - - cmdname = vshCalloc(NULL, (p - rl_line_buffer) + 1, 1); - memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
...because this is a refactoring of pre-existing code.
- - cmd = vshCmddefSearch(cmdname); + cmd = vshDetermineCommandName(); list_index = 0; len = strlen(text); - VIR_FREE(cmdname); }
if (!cmd) @@ -2612,22 +2620,356 @@ vshReadlineOptionsGenerator(const char *text, int state) return NULL; }
My summary from what I gathered after reading the readline programmer's interface: any time a completion is attempted, the generator function will be called repeatedly until it returns NULL; the first call has state==0 and all subsequent calls have non-negative (but otherwise unspecified) state. Each non-NULL return must be a malloc'd string that serves as a possible completion of 'text'.
+/* + * Generator function for command completion, but unlike + * the vshReadlineCommandGenerator which completes command name, this function + * provides more advanced completion for commands by calling specific command + * completers (e.g. vshDomainCompleter). + */ +static char * +vshReadlineCommandCompletionGenerator(const char *text, int state) +{ + static const vshCmdDef *cmd = NULL;
Explicit NULL initialization is not required - all 'static' variables are implicitly zero-initialized.
+ static int list_index, len;
We maintain static state between functions (yuck - readline is not thread-friendly - thankfully, we are only ever using completions from the cli-parsing thread, so I guess we can get away with it).
+ char **completed_names = NULL; + char *name; + + if (!state) { + cmd = vshDetermineCommandName(); + list_index = 0; + len = strlen(text); + }
So on the first call, we determine which command we are completing, and cache the length of 'text' being completed.
+ + if (!cmd) + return NULL;
If we aren't completing a command, we're done (fall back to readline's default completer).
+ + if (!cmd->completer) + return NULL;
If the command doesn't have a specific completer, we're done (fall back to readline's default completer).
+ + completed_names = cmd->completer(cmd->completer_flags);
Malloc a complete list of names. Ouch - that means we malloc the list on EVERY iteration of this function, even though we know this function is called multiple times during one completion attempt. Really, we should only malloc the list when state == 0, and refer back to that list on all other calls. Which means the list of completed_names should be static across calls.
+ + if (!completed_names) + return NULL; + + while ((name = completed_names[list_index])) {
For each name that the helper completer returned (but starting at the point that we remembered from last time around - all the more reason that we should ALSO remember the list from last time around, rather than recomputing the list)...
+ char *res; + list_index++; + + if (STRNEQLEN(name, text, len)) + /* Skip irrelevant names */ + continue;
...if it doesn't match the text we were given, skip it...
+ + res = vshStrdup(NULL, name); + VIR_FREE(name);
Yuck. Why are we malloc'ing a copy and then freeing the original? Why not just do transfer semantics (completed_name[list_index] = NULL, then return name as is, without going through another strdup)?
+ return res; + } + VIR_FREE(completed_names);
Oops - this frees the list of names, but does NOT free the names contained within the list. If ALL such names matched text, then you avoided a leak; but if 'text' is not empty, then you are leaking everywhere that you did a 'continue' in the loop above. Oops - this code is only reached if you return NULL, but completed_names was allocated on every entry (again, my argument is that completed_names should be static, so that it is collected when state==0, and freed when returning NULL, so that all other calls in between reuse the list).
+ + return NULL; +} + +/* + * Generator function for command option completion. Provides advances
s/advances/advanced/
+ * completion for command options. + */ +static char * +vshReadlineOptionsCompletionGenerator(const char *text ATTRIBUTE_UNUSED, + int state ATTRIBUTE_UNUSED) +{ + static const vshCmdDef *cmd = NULL; + static const vshCmdOptDef *opt = NULL; + static int list_index, len; + unsigned long int opt_index = 0; + size_t i; + char **completed_names = NULL;
Similar problems about this list not being static between calls.
+ + for (i = 0; cmd->opts[i].name; i++) { + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name)) &&
Are you sure strstr() is right? Wouldn't STRPREFIX be more appropriate? That is, you only want to complete when you share a common prefix, not when the text is a substring anywhere within a longer name.
+ +/* + * Returns current valid command name present in the rl_line_buffer. + */ +static char * +vshCurrentCmd(void) +{ + const char *name; + const vshCmdGrp *grp; + const vshCmdDef *cmds; + size_t grp_list_index, cmd_list_index; + char *found_cmd = NULL; + char *rl_copy = NULL;
Please do NOT name your local variable rl_copy. rl_* is reserved for readline entry points, and mixing namespaces will only cause confusion for future readers of the code.
+ char *pch;
How often is this function going to be called? Is it something where you should cache the results in something a bit longer-lasting? (Again, I'm not too terribly fond of how thread-unsafe readline is, but as long as it is only used by a single thread doing command-line parsing, we might as well be efficient in our interactions with it)
+ + grp_list_index = 0; + cmd_list_index = 0; + grp = cmdGroups; + + while (grp[grp_list_index].name) { + cmds = grp[grp_list_index].commands; + + if (cmds[cmd_list_index].name) { + while ((name = cmds[cmd_list_index].name)) { + cmd_list_index++; + + if (VIR_STRDUP(rl_copy, rl_line_buffer) < 0) + return NULL; + + char *saveptr; + pch = strtok_r(rl_copy, " ", &saveptr);
Does this work correctly with virsh's quoting rules, which are there to allow for quoting arguments that contain a space?
+ +/* + * Returns current valid command option name present in the rl_line_buffer. + */ +static char * +vshCurrentOpt(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + const char *name; + unsigned long int opt_index = 0; + size_t cmd_opt_list_index; + char *found_opt = NULL; + char *ptr = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->opts) + return NULL; + + cmd_opt_list_index = 0; + + while ((name = cmd->opts[cmd_opt_list_index].name)) { + cmd_opt_list_index++; + + if ((ptr = strstr(rl_line_buffer, name))) {
Again, I'm betting that strstr() is wrong.
+ if (opt_index < (ptr - rl_line_buffer)) { + opt_index = ptr - rl_line_buffer; + if (VIR_STRDUP(found_opt, name) < 0) + return NULL; + } + } + } + + return found_opt; +} + +/* + * Returns name of the command completion currently present in the rl_line_buffer. + */ +static char* +vshCurrentCmdCom(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + size_t i; + char **completed_names = NULL; + char *found_cmd_com = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->completer) + return NULL; + + completed_names = cmd->completer(cmd->completer_flags); + + if (!completed_names) + return NULL; + + for (i = 0; completed_names[i]; i++) { + if (strstr(rl_line_buffer, completed_names[i])) {
Another suspicious strstr.
+ if (VIR_STRDUP(found_cmd_com, completed_names[i]) < 0) + return NULL; + } + } + + return found_cmd_com; +} + +/* + * Returns name of the option completion currently present in the rl_line_buffer. + */ +static char * +vshCurrentOptCom(const char *cmd_name) +{ + const vshCmdDef *cmd = NULL; + const vshCmdOptDef *opt = NULL; + unsigned long int opt_index = 0; + size_t i; + char **completed_names = NULL; + char *found_opt_com = NULL; + char *ptr = NULL; + + if (!cmd_name) + return NULL; + + cmd = vshCmddefSearch(cmd_name); + + if (!cmd) + return NULL; + + if (!cmd->opts) + return NULL; + + for (i = 0; cmd->opts[i].name; i++) { + if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name))) {
Another suspicious strstr.
+ if (opt_index < (ptr - rl_line_buffer)) { + opt_index = ptr - rl_line_buffer; + opt = &cmd->opts[i]; + } + } + } + + if (!opt) + return NULL; + + if (!opt->completer) + return NULL; + + completed_names = opt->completer(opt->completer_flags); + + if (!completed_names) + return NULL; + + for (i = 0; completed_names[i]; i++) { + if (strstr(rl_line_buffer, completed_names[i])) {
and another.
+ if (VIR_STRDUP(found_opt_com, completed_names[i]) < 0) + return NULL; + } + } + + return found_opt_com; +} + static char ** -vshReadlineCompletion(const char *text, int start, - int end ATTRIBUTE_UNUSED) +vshReadlineCompletion(const char *text, int start, int end ATTRIBUTE_UNUSED)
Why the spurious reformat?
{ - char **matches = (char **) NULL; + const char *cmd = vshCurrentCmd(); + const char *cmd_com = vshCurrentCmdCom(cmd); + const char *opt = vshCurrentOpt(cmd); + const char *opt_com = vshCurrentOptCom(cmd); + char **matches = (char **)NULL;
- if (start == 0) - /* command name generator */ + if (start == 0) { + /* Command name generator. */ matches = rl_completion_matches(text, vshReadlineCommandGenerator); - else - /* commands options */ - matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else { + /* Command completer, commands options and commands options completer + * generators. + */ + if (strstr(text, "-")) {
Again, strstr() is probably dead wrong here.
+ /* When user wants to see options. */ + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && !opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
I'm not sure this is quite right. Remember, in my example of 2/6, if the user types 'vol-key <TAB>', the completion should list the UNION of two different completions: all possible options (--vol and --pool, maybe even --help) and all possible volume names that don't resemble options. But I don't see the filtering that rejects a volume name beginning with '-'.
+ } else if (cmd_com && !opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (cmd_com && opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && opt && !opt_com) { + matches = rl_completion_matches(text, vshReadlineCommandCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsCompletionGenerator); + if (!matches) + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (!cmd_com && opt && opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator); + } else if (cmd_com && opt && opt_com) { + matches = rl_completion_matches(text, vshReadlineOptionsGenerator);
This looks like a rat's nest of completion functions, especially, when some of the clauses (like these last two) have identical contents (and could be combined into one by using simply 'if (opt && opt_com)' instead of basing off of !cmd_com/cmd_com). Are we sure this complexity is necessary?
+ } + } + + VIR_FREE(cmd); + VIR_FREE(cmd_com); + VIR_FREE(opt); + VIR_FREE(opt_com); + return matches; }
- static int vshReadlineInit(vshControl *ctl) {
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Function vshDomainCompler returns domains names which can be used by various virsh commands, for example: virsh> start <TAB> fedora domain_foo domain_bar virsh> start f<TAB> virsh> start fedora Michal Privoznik recommended to add global variable virConnectPtr *__my_conn so we can get the list of domains from the virConnecTListAllDomains(). vshReconnect() is called before the first command is executed in order to provide autocompletion for the very first command. --- v2 * global variable __my_conn renamed to vshConn * @name is now const char * * label cleanup renamed to error v3 * removed useless if * used virStringFreeList() instead of iteration * removed vshReconnect() call from main(), reconnecting now takes place in vshReadlineCommandCompletionGenerator() and only for relevant cmds * added vshControl *vshCtl instead of virConnectPtr *vshConn because vshCtl is needed in order to call vshReconnect() * moved all .completer = vshDomainCompleter initializations from other patches into this tools/virsh-domain-monitor.c | 32 +++++-- tools/virsh-domain.c | 221 ++++++++++++++++++++++++++++++++++--------- tools/virsh-snapshot.c | 45 +++++++-- tools/virsh.c | 48 ++++++++++ tools/virsh.h | 2 + 5 files changed, 285 insertions(+), 63 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b29b82a..0f30902 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1876,7 +1876,9 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomBlkError, .opts = opts_domblkerror, .info = info_domblkerror, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domblkinfo", .handler = cmdDomblkinfo, @@ -1888,7 +1890,10 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomblklist, .opts = opts_domblklist, .info = info_domblklist, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domblkstat", .handler = cmdDomblkstat, @@ -1900,7 +1905,9 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomControl, .opts = opts_domcontrol, .info = info_domcontrol, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domif-getlink", .handler = cmdDomIfGetLink, @@ -1912,7 +1919,10 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomiflist, .opts = opts_domiflist, .info = info_domiflist, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domifstat", .handler = cmdDomIfstat, @@ -1924,19 +1934,27 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDominfo, .opts = opts_dominfo, .info = info_dominfo, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "dommemstat", .handler = cmdDomMemStat, .opts = opts_dommemstat, .info = info_dommemstat, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domstate", .handler = cmdDomstate, .opts = opts_domstate, .info = info_domstate, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "list", .handler = cmdList, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5d4913d..5b200a3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10358,7 +10358,10 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdAutostart, .opts = opts_autostart, .info = info_autostart, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "blkdeviotune", .handler = cmdBlkdeviotune, @@ -10370,7 +10373,10 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdBlkiotune, .opts = opts_blkiotune, .info = info_blkiotune, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "blockcommit", .handler = cmdBlockCommit, @@ -10413,7 +10419,10 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdConsole, .opts = opts_console, .info = info_console, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, #endif {.name = "cpu-baseline", @@ -10450,13 +10459,18 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdDesc, .opts = opts_desc, .info = info_desc, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "destroy", .handler = cmdDestroy, .opts = opts_destroy, .info = info_destroy, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "detach-device", .handler = cmdDetachDevice, @@ -10480,31 +10494,45 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdDomDisplay, .opts = opts_domdisplay, .info = info_domdisplay, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domfstrim", .handler = cmdDomFSTrim, .opts = opts_domfstrim, .info = info_domfstrim, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domhostname", .handler = cmdDomHostname, .opts = opts_domhostname, .info = info_domhostname, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domid", .handler = cmdDomid, .opts = opts_domid, .info = info_domid, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domif-setlink", .handler = cmdDomIfSetLink, .opts = opts_domif_setlink, .info = info_domif_setlink, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domiftune", .handler = cmdDomIftune, @@ -10522,7 +10550,9 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdDomjobinfo, .opts = opts_domjobinfo, .info = info_domjobinfo, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domname", .handler = cmdDomname, @@ -10534,19 +10564,27 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdDomPMSuspend, .opts = opts_dom_pm_suspend, .info = info_dom_pm_suspend, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING }, {.name = "dompmwakeup", .handler = cmdDomPMWakeup, .opts = opts_dom_pm_wakeup, .info = info_dom_pm_wakeup, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "domuuid", .handler = cmdDomuuid, .opts = opts_domuuid, .info = info_domuuid, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "domxml-from-native", .handler = cmdDomXMLFromNative, @@ -10564,31 +10602,46 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdDump, .opts = opts_dump, .info = info_dump, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING }, {.name = "dumpxml", .handler = cmdDumpXML, .opts = opts_dumpxml, .info = info_dumpxml, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "edit", .handler = cmdEdit, .opts = opts_edit, .info = info_edit, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "inject-nmi", .handler = cmdInjectNMI, .opts = opts_inject_nmi, .info = info_inject_nmi, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING }, {.name = "send-key", .handler = cmdSendKey, .opts = opts_send_key, .info = info_send_key, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING }, {.name = "send-process-signal", .handler = cmdSendProcessSignal, @@ -10600,19 +10653,29 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdLxcEnterNamespace, .opts = opts_lxc_enter_namespace, .info = info_lxc_enter_namespace, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "managedsave", .handler = cmdManagedSave, .opts = opts_managedsave, .info = info_managedsave, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING | + VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE }, {.name = "managedsave-remove", .handler = cmdManagedSaveRemove, .opts = opts_managedsaveremove, .info = info_managedsaveremove, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE }, {.name = "maxvcpus", .handler = cmdMaxvcpus, @@ -10624,7 +10687,10 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdMemtune, .opts = opts_memtune, .info = info_memtune, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "migrate", .handler = cmdMigrate, @@ -10636,31 +10702,46 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdMigrateSetMaxDowntime, .opts = opts_migrate_setmaxdowntime, .info = info_migrate_setmaxdowntime, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "migrate-compcache", .handler = cmdMigrateCompCache, .opts = opts_migrate_compcache, .info = info_migrate_compcache, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_RUNNING }, {.name = "migrate-setspeed", .handler = cmdMigrateSetMaxSpeed, .opts = opts_migrate_setspeed, .info = info_migrate_setspeed, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "migrate-getspeed", .handler = cmdMigrateGetMaxSpeed, .opts = opts_migrate_getspeed, .info = info_migrate_getspeed, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "numatune", .handler = cmdNumatune, .opts = opts_numatune, .info = info_numatune, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "qemu-attach", .handler = cmdQemuAttach, @@ -10672,25 +10753,35 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdQemuMonitorCommand, .opts = opts_qemu_monitor_command, .info = info_qemu_monitor_command, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "qemu-agent-command", .handler = cmdQemuAgentCommand, .opts = opts_qemu_agent_command, .info = info_qemu_agent_command, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "reboot", .handler = cmdReboot, .opts = opts_reboot, .info = info_reboot, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "reset", .handler = cmdReset, .opts = opts_reset, .info = info_reset, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "restore", .handler = cmdRestore, @@ -10702,7 +10793,10 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdResume, .opts = opts_resume, .info = info_resume, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_PAUSED }, {.name = "save", .handler = cmdSave, @@ -10732,61 +10826,83 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdSchedinfo, .opts = opts_schedinfo, .info = info_schedinfo, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "screenshot", .handler = cmdScreenshot, .opts = opts_screenshot, .info = info_screenshot, - .flags = 0 + .flags = 0, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "setmaxmem", .handler = cmdSetmaxmem, .opts = opts_setmaxmem, .info = info_setmaxmem, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "setmem", .handler = cmdSetmem, .opts = opts_setmem, .info = info_setmem, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "setvcpus", .handler = cmdSetvcpus, .opts = opts_setvcpus, .info = info_setvcpus, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "shutdown", .handler = cmdShutdown, .opts = opts_shutdown, .info = info_shutdown, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "start", .handler = cmdStart, .opts = opts_start, .info = info_start, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_INACTIVE | + VIR_CONNECT_LIST_DOMAINS_PERSISTENT }, {.name = "suspend", .handler = cmdSuspend, .opts = opts_suspend, .info = info_suspend, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "ttyconsole", .handler = cmdTTYConsole, .opts = opts_ttyconsole, .info = info_ttyconsole, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = "undefine", .handler = cmdUndefine, .opts = opts_undefine, .info = info_undefine, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "update-device", .handler = cmdUpdateDevice, @@ -10804,25 +10920,36 @@ const vshCmdDef domManagementCmds[] = { .handler = cmdVcpuinfo, .opts = opts_vcpuinfo, .info = info_vcpuinfo, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "vcpupin", .handler = cmdVcpuPin, .opts = opts_vcpupin, .info = info_vcpupin, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "emulatorpin", .handler = cmdEmulatorPin, .opts = opts_emulatorpin, .info = info_emulatorpin, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "vncdisplay", .handler = cmdVNCDisplay, .opts = opts_vncdisplay, .info = info_vncdisplay, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE }, {.name = NULL} }; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e37a5b3..77f9273 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -2006,25 +2006,37 @@ const vshCmdDef snapshotCmds[] = { .handler = cmdSnapshotCreate, .opts = opts_snapshot_create, .info = info_snapshot_create, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-create-as", .handler = cmdSnapshotCreateAs, .opts = opts_snapshot_create_as, .info = info_snapshot_create_as, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-current", .handler = cmdSnapshotCurrent, .opts = opts_snapshot_current, .info = info_snapshot_current, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-delete", .handler = cmdSnapshotDelete, .opts = opts_snapshot_delete, .info = info_snapshot_delete, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-dumpxml", .handler = cmdSnapshotDumpXML, @@ -2036,31 +2048,46 @@ const vshCmdDef snapshotCmds[] = { .handler = cmdSnapshotEdit, .opts = opts_snapshot_edit, .info = info_snapshot_edit, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-info", .handler = cmdSnapshotInfo, .opts = opts_snapshot_info, .info = info_snapshot_info, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-list", .handler = cmdSnapshotList, .opts = opts_snapshot_list, .info = info_snapshot_list, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-parent", .handler = cmdSnapshotParent, .opts = opts_snapshot_parent, .info = info_snapshot_parent, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = "snapshot-revert", .handler = cmdDomainSnapshotRevert, .opts = opts_snapshot_revert, .info = info_snapshot_revert, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE }, {.name = NULL} }; diff --git a/tools/virsh.c b/tools/virsh.c index 5be8e7e..ae2dd55 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -88,6 +88,8 @@ static char *progname; static const vshCmdGrp cmdGroups[]; +vshControl *vshCtl; + /* Bypass header poison */ #undef strdup @@ -317,6 +319,7 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, static void vshReconnect(vshControl *ctl) { + printf("\n>>>\n"); bool connected = false; if (ctl->conn) { @@ -2510,6 +2513,46 @@ vshCloseLogFile(vshControl *ctl) #ifdef USE_READLINE +/* ------------- + * Completers + * ------------- + */ + +char ** +vshDomainCompleter(unsigned int flags) +{ + virDomainPtr *domains; + size_t i; + char **names = NULL; + int ndomains; + + if (!vshCtl->conn) + return NULL; + + ndomains = virConnectListAllDomains(vshCtl->conn, &domains, flags); + + if (ndomains < 0) + return NULL; + + names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1)); + + for (i = 0; i < ndomains; i++) { + const char *name = virDomainGetName(domains[i]); + if (VIR_STRDUP(names[i], name) < 0) { + virDomainFree(domains[i]); + goto error; + } + virDomainFree(domains[i]); + } + names[i] = NULL; + VIR_FREE(domains); + return names; + +error: + virStringFreeList(names); + return NULL; +} + /* ----------------- * Readline stuff * ----------------- @@ -2643,6 +2686,10 @@ vshReadlineCommandCompletionGenerator(const char *text, int state) if (!cmd) return NULL; + if ((vshCtl->conn == NULL || disconnected) && + !(cmd->flags & VSH_CMD_FLAG_NOCONNECT)) + vshReconnect(vshCtl); + if (!cmd->completer) return NULL; @@ -3509,6 +3556,7 @@ main(int argc, char **argv) ctl->debug = VSH_DEBUG_DEFAULT; ctl->escapeChar = "^]"; /* Same default as telnet */ + vshCtl = ctl; if (!setlocale(LC_ALL, "")) { perror("setlocale"); diff --git a/tools/virsh.h b/tools/virsh.h index 064acde..68414e4 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -255,6 +255,8 @@ struct _vshCmdGrp { const vshCmdDef *commands; }; +char **vshDomainCompleter(unsigned int flags); + void vshError(vshControl *ctl, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); void vshOpenLogFile(vshControl *ctl); -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
Function vshDomainCompler returns domains names which can be used by various virsh commands, for example:
virsh> start <TAB> fedora domain_foo domain_bar virsh> start f<TAB> virsh> start fedora
Michal Privoznik recommended to add global variable virConnectPtr *__my_conn
Stale name in the commit message.
so we can get the list of domains from the virConnecTListAllDomains().
vshReconnect() is called before the first command is executed in order to provide autocompletion for the very first command.
Stale comment, now that you fixed it to delay until completion requires a connection.
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b29b82a..0f30902 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1876,7 +1876,9 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomBlkError, .opts = opts_domblkerror, .info = info_domblkerror, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
I'm still not sure I get why we need per-command completers; isn't per-option completion sufficient (by associating the completer with the --domain of each of these commands)?
@@ -1888,7 +1890,10 @@ const vshCmdDef domMonitoringCmds[] = { .handler = cmdDomblklist, .opts = opts_domblklist, .info = info_domblklist, - .flags = 0 + .flags = 0, + .completer = vshDomainCompleter, + .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_INACTIVE
Style: please use trailing comma after the last element of the struct, so that future additions don't have to modify existing lines when adding even more struct members down the road (throughout this patch, and probably throughout the series). Yeah, I know existing C99 initializers for vshCmdInfo.dad and vshCmdOptDef.help aren't using trailing commas consistently, so maybe that's worth a separate cleanup patch. If we had been following that style, you wouldn't be modifying the .flags lines.
+++ b/tools/virsh.c @@ -88,6 +88,8 @@ static char *progname;
static const vshCmdGrp cmdGroups[];
+vshControl *vshCtl;
This variable should probably be marked static, as I don't see it referenced in any other file.
+ /* Bypass header poison */ #undef strdup
@@ -317,6 +319,7 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, static void vshReconnect(vshControl *ctl) { + printf("\n>>>\n");
Leftover debugging?
@@ -2510,6 +2513,46 @@ vshCloseLogFile(vshControl *ctl)
#ifdef USE_READLINE
+/* ------------- + * Completers + * ------------- + */ + +char ** +vshDomainCompleter(unsigned int flags)
Ouch - you have a link error if building without USE_READLINE, since the use of this function was unconditional but its implementation is conditional. How much of this code can you hoist outside of USE_READLINE? If you can't, then please provide the #else half so that at least things still link.
+{ + virDomainPtr *domains; + size_t i; + char **names = NULL; + int ndomains; + + if (!vshCtl->conn) + return NULL; + + ndomains = virConnectListAllDomains(vshCtl->conn, &domains, flags); + + if (ndomains < 0) + return NULL; + + names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1)); +
VIR_ALLOC_N seems nicer for the task.
+ for (i = 0; i < ndomains; i++) { + const char *name = virDomainGetName(domains[i]); + if (VIR_STRDUP(names[i], name) < 0) { + virDomainFree(domains[i]); + goto error; + } + virDomainFree(domains[i]); + } + names[i] = NULL; + VIR_FREE(domains); + return names; + +error:
Memory leak - if you get here, you don't finish calling virDomainFree() on the tail of the list, nor VIR_FREE(domains). That cleanup needs to be unconditional. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
vshSuspendTargetCompleter returns targets available for suspend. This completer can be used for the command option completion (for dompmsuspend, etc.). virsh> dompmsuspend --target <TAB> mem disk hybrid virsh> dompmsuspend --target h<TAB> virsh> dompmsuspend --target hybrid --- v2 * label cleanup renamed to error * vshSuspendTargetCompleter added to opts_dom_pm_suspend v3 * removed useless if * used virStringFreeList() instead of iteration tools/virsh-domain.c | 3 ++- tools/virsh.c | 23 +++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5b200a3..8a118ed 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2756,7 +2756,8 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { .flags = VSH_OFLAG_REQ, .help = N_("mem(Suspend-to-RAM), " "disk(Suspend-to-Disk), " - "hybrid(Hybrid-Suspend)") + "hybrid(Hybrid-Suspend)"), + .completer = vshSuspendTargetCompleter }, {.name = NULL} }; diff --git a/tools/virsh.c b/tools/virsh.c index ae2dd55..5a5e520 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2553,6 +2553,29 @@ error: return NULL; } +char ** +vshSuspendTargetCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED) +{ + const char *targets[] = {"mem", "disk", "hybrid"}; + const unsigned int targets_size = ARRAY_CARDINALITY(targets); + char **names = NULL; + size_t i; + + names = vshMalloc(NULL, sizeof(char *) * (targets_size + 1)); + + for (i = 0; i < targets_size; i++) { + if (VIR_STRDUP(names[i], targets[i]) < 0) + goto error; + } + + names[i] = NULL; + return names; + +error: + virStringFreeList(names); + return NULL; +} + /* ----------------- * Readline stuff * ----------------- diff --git a/tools/virsh.h b/tools/virsh.h index 68414e4..6767e65 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -256,6 +256,7 @@ struct _vshCmdGrp { }; char **vshDomainCompleter(unsigned int flags); +char **vshSuspendTargetCompleter(unsigned int unused_flags); void vshError(vshControl *ctl, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
vshSuspendTargetCompleter returns targets available for suspend.
This completer can be used for the command option completion (for dompmsuspend, etc.).
virsh> dompmsuspend --target <TAB> mem disk hybrid virsh> dompmsuspend --target h<TAB> virsh> dompmsuspend --target hybrid --- v2 * label cleanup renamed to error * vshSuspendTargetCompleter added to opts_dom_pm_suspend
v3 * removed useless if * used virStringFreeList() instead of iteration
tools/virsh-domain.c | 3 ++- tools/virsh.c | 23 +++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 26 insertions(+), 1 deletion(-)
This patch looks pretty decent, especially since it is installed as a per-option handler.
+char ** +vshSuspendTargetCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED) +{ + const char *targets[] = {"mem", "disk", "hybrid"}; + const unsigned int targets_size = ARRAY_CARDINALITY(targets); + char **names = NULL; + size_t i; + + names = vshMalloc(NULL, sizeof(char *) * (targets_size + 1)); + + for (i = 0; i < targets_size; i++) { + if (VIR_STRDUP(names[i], targets[i]) < 0) + goto error; + } + + names[i] = NULL; + return names;
Call me a hacker, but what's wrong with this equivalent implementation done by reusing existing code: char ** vshSuspendTargetCompleter(unsigned int ignored ATTRIBUTE_UNUSED) { return virStringSplit("mem disk hybrid", " ", 0); } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 26/08/13 at 02:43pm, Eric Blake wrote:
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
vshSuspendTargetCompleter returns targets available for suspend.
This completer can be used for the command option completion (for dompmsuspend, etc.).
virsh> dompmsuspend --target <TAB> mem disk hybrid virsh> dompmsuspend --target h<TAB> virsh> dompmsuspend --target hybrid --- v2 * label cleanup renamed to error * vshSuspendTargetCompleter added to opts_dom_pm_suspend
v3 * removed useless if * used virStringFreeList() instead of iteration
tools/virsh-domain.c | 3 ++- tools/virsh.c | 23 +++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 26 insertions(+), 1 deletion(-)
This patch looks pretty decent, especially since it is installed as a per-option handler.
+char ** +vshSuspendTargetCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED) +{ + const char *targets[] = {"mem", "disk", "hybrid"}; + const unsigned int targets_size = ARRAY_CARDINALITY(targets); + char **names = NULL; + size_t i; + + names = vshMalloc(NULL, sizeof(char *) * (targets_size + 1)); + + for (i = 0; i < targets_size; i++) { + if (VIR_STRDUP(names[i], targets[i]) < 0) + goto error; + } + + names[i] = NULL; + return names;
Call me a hacker, but what's wrong with this equivalent implementation done by reusing existing code:
char ** vshSuspendTargetCompleter(unsigned int ignored ATTRIBUTE_UNUSED) { return virStringSplit("mem disk hybrid", " ", 0); }
Yeah, this is much better. I will definitely use this. -- Tomas Meszaros
vshRebootShutdownModeCompleter returns available shutdown mode names. This can be used for --mode auto completion for commands such as reboot or shutdown. for example: virsh> reboot --mode <TAB> acpi agent initctl signal virsh> reboot --mode i<TAB> virsh> reboot --mode initctl --- v3 * removed useless if * used virStringFreeList() instead of iteration * moved all .completer = vshRebootShutdownModeCompleter initializations into this patch tools/virsh-domain.c | 9 +++++---- tools/virsh.c | 23 +++++++++++++++++++++++ tools/virsh.h | 1 + 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8a118ed..c60a01c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2756,8 +2756,7 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { .flags = VSH_OFLAG_REQ, .help = N_("mem(Suspend-to-RAM), " "disk(Suspend-to-Disk), " - "hybrid(Hybrid-Suspend)"), - .completer = vshSuspendTargetCompleter + "hybrid(Hybrid-Suspend)") }, {.name = NULL} }; @@ -4714,7 +4713,8 @@ static const vshCmdOptDef opts_shutdown[] = { }, {.name = "mode", .type = VSH_OT_STRING, - .help = N_("shutdown mode: acpi|agent|initctl|signal") + .help = N_("shutdown mode: acpi|agent|initctl|signal"), + .completer = vshRebootShutdownModeCompleter }, {.name = NULL} }; @@ -4800,7 +4800,8 @@ static const vshCmdOptDef opts_reboot[] = { }, {.name = "mode", .type = VSH_OT_STRING, - .help = N_("shutdown mode: acpi|agent|initctl|signal") + .help = N_("shutdown mode: acpi|agent|initctl|signal"), + .completer = vshRebootShutdownModeCompleter }, {.name = NULL} }; diff --git a/tools/virsh.c b/tools/virsh.c index 5a5e520..e6b1309 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2576,6 +2576,29 @@ error: return NULL; } +char ** +vshRebootShutdownModeCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED) +{ + const char *modes[] = {"acpi", "agent", "initctl", "signal"}; + const unsigned int modes_size = ARRAY_CARDINALITY(modes); + char **names = NULL; + size_t i; + + names = vshMalloc(NULL, sizeof(char *) * (modes_size + 1)); + + for (i = 0; i < modes_size; i++) { + if (VIR_STRDUP(names[i], modes[i]) < 0) + goto cleanup; + } + + names[i] = NULL; + return names; + +cleanup: + virStringFreeList(names); + return NULL; +} + /* ----------------- * Readline stuff * ----------------- diff --git a/tools/virsh.h b/tools/virsh.h index 6767e65..61510b0 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -257,6 +257,7 @@ struct _vshCmdGrp { char **vshDomainCompleter(unsigned int flags); char **vshSuspendTargetCompleter(unsigned int unused_flags); +char **vshRebootShutdownModeCompleter(unsigned int unused_flags); void vshError(vshControl *ctl, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); -- 1.8.3.1
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
vshRebootShutdownModeCompleter returns available shutdown mode names. This can be used for --mode auto completion for commands such as reboot or shutdown.
for example:
virsh> reboot --mode <TAB> acpi agent initctl signal virsh> reboot --mode i<TAB> virsh> reboot --mode initctl --- v3 * removed useless if * used virStringFreeList() instead of iteration * moved all .completer = vshRebootShutdownModeCompleter initializations into this patch
Once again, seems reasonable, but can be shortened:
+char ** +vshRebootShutdownModeCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED) +{ + const char *modes[] = {"acpi", "agent", "initctl", "signal"}; + const unsigned int modes_size = ARRAY_CARDINALITY(modes); + char **names = NULL; + size_t i; + + names = vshMalloc(NULL, sizeof(char *) * (modes_size + 1)); + + for (i = 0; i < modes_size; i++) { + if (VIR_STRDUP(names[i], modes[i]) < 0) + goto cleanup; + } + + names[i] = NULL; + return names; + +cleanup: + virStringFreeList(names); + return NULL;
All this boils down to an open-coded version of return virStringSplit("acpi agent initctl signal", " ", 0); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/26/2013 06:36 AM, Tomas Meszaros wrote:
Hi, this patch series is a prototype for my GSoC project (Michal Privoznik is my mentor).
I'm working on virsh auto-completion, trying to make it more "intelligent". At this stage, prototype is capable of command and option completion. Three completer functions are currently implemented so you can test it. If it turns out that this prototype is good enough, I will implement more completer functions.
As we just discovered on IRC, the formula for building without readline is to either uninstall readline-devel, or to build with: ./configure ac_cv_lib_readline_readline=no \ ac_cv_search_tgetent=no ac_cv_lib_readline_rl_initialize=no (Yes, I need to add a patch that adds './configure --disable-readline'; I started a patch on my local tree when we were trying to resolve the incompatible vbox (LGPLv2-only) vs. readline (GPLv3+) issue a while back, but since we solved it via moving vbox, I didn't have to castrate readline at that time) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Tomas Meszaros