GNOME Bugzilla – Bug 729846
cli: readline library usage in novice mode to allow repeating and editing incorrect commands
Last modified: 2014-06-04 08:27:23 UTC
jklimes says: nmcli started using readline library with the nmcli interactive editor feature. However, we tried to make the use of a edit-line library optional. Mainly due to minimal/embedded installations. An edit-line library is only loaded at runtime when it is available (libreadline, libedit). Else plain (not edit-line) mode is used as a fallback. But, now it seems that libreadline is almost ubiquitous. So I plan to make libreadline hard build dependency and rewrite the code: It will bring 2 main advantages: * libreadline can be used in all places (not just in the editor) - this bug * the code can be simplified
Changes for readline usage in nmcli implemented in jk/nmcli-readline branch. Highlights: * nmcli is linked with libreadline (built-time dependency) * using readline in majority of commands asking for input (that is when --ask option is used) * add a bunch of TAB completion stuff for various options
>> cli: move readline related functions to common.h to be usable throughout nmcli does this only move code around and rename it? Could you mention in the commit message, if this is (really) a trivial patch that does not change functionality. >> cli: use readline in other commands (besides interactive editor) duplicated function with different names in the same file: +static char ** +nmcli_add_tab_completion (char *text, int start, int end) +static char ** +nmcli_con_tab_completion (char *text, int start, int end) in another file: +static char ** +nmcli_device_tab_completion (char *text, int start, int end) How about moving them to cli/src/common.c? Then you also don't need: #include <readline/readline.h> >> cli: implement TAB completion for inputs in questionnaire mode +static const char *nmc_valid_vpns[] = + { "openvpn", "vpnc", "pptp", "openconnect", "openswan", NULL }; Related to bug https://bugzilla.redhat.com/show_bug.cgi?id=1100750 +/* Global variable defined in nmcli.c - used for TAB completion */ +extern NmCli nm_cli; why no header? I think it belongs to cli/src/common.[hc] + if (n3 == 0)<<TRAILING WHITE SPACE>> + return TRUE; +static gboolean +is_single_word (const char* line) this does accept leading whitespace, but no trailing whitespace. Is that intended? Shouldn't it be: + if (n3 == 0) - if (line[n2+n3] == 0) ? Also, this accepts the empty-word and only-ws. Is this intended? Otherwise it should be: + if (n3 == 0) - if (line[n2+n3] == 0 && n2 > n1) ? I dislike all this duplication of metadata about settings. E.g. the gen_func_bond_mode() needs the same information as already inside libnm-util and NMDeviceBond. libnm-util should expose better metadata of the settings. >> cli: implement TAB completion for connection and interface names is_single_word() to cli/src/common.[hc]? +/* Global variable defined in nmcli.c */ +extern NmCli nm_cli;
Addressed the comments and rebased to master. (In reply to comment #2) > > >> cli: move readline related functions to common.h to be usable throughout nmcli > > does this only move code around and rename it? Could you mention in the commit > message, if this is (really) a trivial patch that does not change > functionality. > The message mentioned that it moves and renames the functions. Anyway, I highlighted that it makes no actual changes. > > >> cli: use readline in other commands (besides interactive editor) > > duplicated function with different names > > in the same file: > > +static char ** > +nmcli_add_tab_completion (char *text, int start, int end) > > +static char ** > +nmcli_con_tab_completion (char *text, int start, int end) > > > in another file: > > +static char ** > +nmcli_device_tab_completion (char *text, int start, int end) > > > How about moving them to cli/src/common.c? > Then you also don't need: > #include <readline/readline.h> > The functions are enhanced in later commits; they are not common and belong to the particular files. I've renamed nmcli_add_tab_completion() to nmcli_con_add_tab_completion() to make it more clear that handles completion for 'nmcli con add' > > >> cli: implement TAB completion for inputs in questionnaire mode > > +static const char *nmc_valid_vpns[] = > + { "openvpn", "vpnc", "pptp", "openconnect", "openswan", NULL }; > > Related to bug https://bugzilla.redhat.com/show_bug.cgi?id=1100750 > Sure, rebased to master and fixed. > > + if (n3 == 0)<<TRAILING WHITE SPACE>> > + return TRUE; > Fixed. > > +static gboolean > +is_single_word (const char* line) > > this does accept leading whitespace, but no trailing whitespace. Is that > intended? Shouldn't it be: > + if (n3 == 0) > - if (line[n2+n3] == 0) ? > Also, this accepts the empty-word and only-ws. Is this intended? > Otherwise it should be: > + if (n3 == 0) > - if (line[n2+n3] == 0 && n2 > n1) ? > It should not accept trailing spaces. It is used for stopping completions when a completion was done. For example: nmcli -c dev disconnect<Enter> Interface: et<TAB> Interface: eth0 <TAB> don't complete any more > > >> cli: implement TAB completion for connection and interface names > > is_single_word() to cli/src/common.[hc]? > The function happens to be the same now, but it is not any general purpose one and it may be adjusted later for particular cases. > +/* Global variable defined in nmcli.c */ > +extern NmCli nm_cli; We could declare "extern NmCli nm_cli"; in nmcli.h and define it in nmcli.c. But I would leave that for later refactoring of NmCli object usage - making it a sigleton and use it directly everywhere. (Now a pointer to NmCli is passed to almost all functions.)
> cli/build: make libreadline a build-time dependency (bgo #729846) >+AC_CHECK_LIB(readline, readline, [READLINE_LIBS=-lreadline; READLINE_CFLAGS=-I/usr/include/readline], [AC_MSG_ERROR(readline library is required)]) >+#include <readline/readline.h> The headers are, eg, /usr/include/readline/readline.h, so that READLINE_CFLAGS is wrong; you just need -I/usr/include, and then "#include <readline/readline.h>" will work. (But you don't actually want "READLINE_CFLAGS=-I/usr/include", because adding -I/usr/include explicitly is bad.) > cli: use readline in other commands (besides interactive editor) nmcli_con_add_tab_completion() is unnecessary isn't it, because nmcli_con_tab_completion() would already have been set? (Oh, I see, it gets changed in the next commit. Maybe it shouldn't be added until there.) > cli: implement TAB completion for inputs in questionnaire mode Lots of places that do yes/no prompts don't call normalize_yes_no(), and I can't tell if there's some reason, or it's just an oversight. >+ printf (_("Some optional arguments (%d) are available for '%s' connection type.\n"), num, type); Use ngettext: printf (ngettext ("There is %d optional argument for '%s' connection type.\n", "There are %d optional arguments for '%s' connection type.\n", num), num, type); >+ char *prompt_mode = g_strdup_printf (_("Transport mode %s"), PROMPT_IB_MODE); ... >+ *mode = nmc_readline (prompt_mode, TRUE); ... >+ g_free (prompt_mode); Might simplify things to make nmc_readline() have printf-like args? (It appears that you never actually pass FALSE for add_to_history anyway, so you could just drop that argument.) Or, it might even be useful to have a wrapper that takes a base prompt string, the const char *[] of possibilities, and the default value, builds a "(%s/%s/%s) [%s]" string, prompts via nmc_readline(), and then calls nmc_string_is_valid() on the result before returning it.
(In reply to comment #4) > > cli/build: make libreadline a build-time dependency (bgo #729846) > > >+AC_CHECK_LIB(readline, readline, [READLINE_LIBS=-lreadline; READLINE_CFLAGS=-I/usr/include/readline], [AC_MSG_ERROR(readline library is required)]) > > >+#include <readline/readline.h> > > The headers are, eg, /usr/include/readline/readline.h, so that READLINE_CFLAGS > is wrong; you just need -I/usr/include, and then "#include > <readline/readline.h>" will work. (But you don't actually want > "READLINE_CFLAGS=-I/usr/include", because adding -I/usr/include explicitly is > bad.) > READLINE_CFLAGS removed. > > > cli: use readline in other commands (besides interactive editor) > > nmcli_con_add_tab_completion() is unnecessary isn't it, because > nmcli_con_tab_completion() would already have been set? (Oh, I see, it gets > changed in the next commit. Maybe it shouldn't be added until there.) > Yeah, the function is extended in the next commit. This commit just use it for disabling default filename completion. > > > cli: implement TAB completion for inputs in questionnaire mode > > Lots of places that do yes/no prompts don't call normalize_yes_no(), and I > can't tell if there's some reason, or it's just an oversight. > We don't want to use the function on all places. I added a new commit to use readline for quit confirmation (and don't use normalize_yes_no() here because I only want (localized) yes/no there). > >+ printf (_("Some optional arguments (%d) are available for '%s' connection type.\n"), num, type); > > Use ngettext: > > printf (ngettext ("There is %d optional argument for '%s' connection > type.\n", > "There are %d optional arguments for '%s' connection > type.\n", > num), > num, type); > Done. > > >+ char *prompt_mode = g_strdup_printf (_("Transport mode %s"), PROMPT_IB_MODE); > ... > >+ *mode = nmc_readline (prompt_mode, TRUE); > ... > >+ g_free (prompt_mode); > > Might simplify things to make nmc_readline() have printf-like args? (It appears > that you never actually pass FALSE for add_to_history anyway, so you could just > drop that argument.) > > Or, it might even be useful to have a wrapper that takes a base prompt string, > the const char *[] of possibilities, and the default value, builds a > "(%s/%s/%s) [%s]" string, prompts via nmc_readline(), and then calls > nmc_string_is_valid() on the result before returning it. Changed nmc_readline() to accept printf-like variable arguments. I have also removed add_to_history parameter. (I had envisioned before we might not want to store all values to history. But we store them currently). Branch rebased to master.
> cli: make prompt argument a printf-like format string, remove history argument Could add some extra compiler-level argument checking by doing: +char *nmc_readline (const char *prompt_fmt, ...) G_GNUC_PRINTF (1, 2); Beyond that, looks great to me.
(In reply to comment #6) > > cli: make prompt argument a printf-like format string, remove history argument > > Could add some extra compiler-level argument checking by doing: > > +char *nmc_readline (const char *prompt_fmt, ...) G_GNUC_PRINTF (1, 2); > Done. Commits in master: 13b1060 Merge changes for readline support in nmcli (bgo #729846) 82db87a cli: use readline for quit confirmation in editor, and accept localized answer 00c700b cli: implement TAB completion for connection and interface names bfb1200 cli: implement TAB completion for inputs in questionnaire mode 480f69e cli: use readline also in other commands (besides interactive editor) b71af5b cli: make prompt argument a printf-like format string, remove history argument a3d89d6 cli: move readline related functions to common.h to be usable throughout nmcli 03be41d cli/build: make libreadline a build-time dependency (bgo #729846)