Re: [libvirt] [PATCH 08/11] tools: Provide bash autompletion file

On Wed, Nov 08, 2017 at 04:09:10PM +0100, Michal Privoznik wrote: > On 11/08/2017 03:46 PM, Martin Kletzander wrote: >> On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote: >>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>> --- >>> configure.ac               |  3 ++ >>> libvirt.spec.in            |  2 ++ >>> m4/virt-bash-completion.m4 | 74 >>> ++++++++++++++++++++++++++++++++++++++++++++++ >>> tools/Makefile.am          | 22 ++++++++++++-- >>> tools/bash-completion/vsh  | 73 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 172 insertions(+), 2 deletions(-) >>> create mode 100644 m4/virt-bash-completion.m4 >>> create mode 100644 tools/bash-completion/vsh >>> >>> diff --git a/configure.ac b/configure.ac >>> index b2d991c3b..9103612bb 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR >>> LIBVIRT_ARG_ATTR >>> LIBVIRT_ARG_AUDIT >>> LIBVIRT_ARG_AVAHI >>> +LIBVIRT_ARG_BASH_COMPLETION >>> LIBVIRT_ARG_BLKID >>> LIBVIRT_ARG_CAPNG >>> LIBVIRT_ARG_CURL >>> @@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC >>> LIBVIRT_CHECK_ATTR >>> LIBVIRT_CHECK_AUDIT >>> LIBVIRT_CHECK_AVAHI >>> +LIBVIRT_CHECK_BASH_COMPLETION >>> LIBVIRT_CHECK_BLKID >>> LIBVIRT_CHECK_CAPNG >>> LIBVIRT_CHECK_CURL >>> @@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR >>> LIBVIRT_RESULT_ATTR >>> LIBVIRT_RESULT_AUDIT >>> LIBVIRT_RESULT_AVAHI >>> +LIBVIRT_RESULT_BASH_COMPLETION >>> LIBVIRT_RESULT_BLKID >>> LIBVIRT_RESULT_CAPNG >>> LIBVIRT_RESULT_CURL >>> diff --git a/libvirt.spec.in b/libvirt.spec.in >>> index b00689cab..67bbd128c 100644 >>> --- a/libvirt.spec.in >>> +++ b/libvirt.spec.in >>> @@ -2043,6 +2043,8 @@ exit 0 >>> %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp >>> %{_datadir}/systemtap/tapset/libvirt_functions.stp >>> >>> +%{_datadir}/bash-completion/completions/vsh >>> + >>> >>> %if %{with_systemd} >>> %{_unitdir}/libvirt-guests.service >>> diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4 >>> new file mode 100644 >>> index 000000000..e1ef58740 >>> --- /dev/null >>> +++ b/m4/virt-bash-completion.m4 >>> @@ -0,0 +1,74 @@ >>> +dnl Bash completion support >>> +dnl >>> +dnl Copyright (C) 2017 Red Hat, Inc. >>> +dnl >>> +dnl This library is free software; you can redistribute it and/or >>> +dnl modify it under the terms of the GNU Lesser General Public >>> +dnl License as published by the Free Software Foundation; either >>> +dnl version 2.1 of the License, or (at your option) any later version. >>> +dnl >>> +dnl This library is distributed in the hope that it will be useful, >>> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of >>> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU >>> +dnl Lesser General Public License for more details. >>> +dnl >>> +dnl You should have received a copy of the GNU Lesser General Public >>> +dnl License along with this library.  If not, see >>> +dnl <http://www.gnu.org/licenses/>;. >>> +dnl >>> +dnl Inspired by libguestfs code. >>> +dnl >>> + >>> +AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[ >>> +  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion], >>> [check], [2.0]) >>> +  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR], >>> +                   [directory containing bash completions scripts], >>> +                   [check]) >>> +]) >>> + >>> +AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [ >>> +  AC_REQUIRE([LIBVIRT_CHECK_READLINE]) >>> + >>> +  if test "x$with_readline" != "xyes" ; then >>> +    if test "x$with_bash_completion" != "xyes" ; then >>> +      with_bash_completion=no >>> +    else >>> +      AC_MSG_ERROR([readline is required for bash completion support]) >>> +    fi >>> +  else >>> +    if test "x$with_bash_completion" = "xcheck" ; then >>> +      with_bash_completion=yes >>> +    fi >> >> You should change the "check" to "yes" only after everything below >> succeeded. >> >>> +  fi >>> + >>> +  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0]) >>> + >>> +  if test "x$with_bash_completion" = "xyes" ; then >>> +    if test "x$with_bash_completions_dir" = "xcheck"; then >>> +      AC_MSG_CHECKING([for bash-completions directory]) >>> +      BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir >>> bash-completion)" >>> +      AC_MSG_RESULT([$BASH_COMPLETIONS_DIR]) >>> + >>> +      dnl Replace bash completions's exec_prefix with our own. >>> +      dnl Note that ${exec_prefix} is kept verbatim at this point in >>> time, >>> +      dnl and will only be expanded later, when make is called: this >>> makes >>> +      dnl it possible to override such prefix at compilation or >>> installation >>> +      dnl time >>> +      bash_completions_prefix="$($PKG_CONFIG --variable=prefix >>> bash-completion)" >>> +      if test "x$bash_completions_prefix" = "x" ; then >>> +        bash_completions_prefix="/usr" >>> +      fi >>> + >>> +      >>> BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}" >>> >>> >>> +    elif test "x$with_bash_completions_dir" = "xno" || test >>> "x$with_bash_completions_dir" = "xyes"; then >>> +      AC_MSG_ERROR([bash-completions-dir must be used only with valid >>> path]) >> >> Otherwise you can error out here ^^ even when nobody requested anything. >> >>> +    else >>> +      BASH_COMPLETIONS_DIR=$with_bash_completions_dir >>> +    fi >>> +    AC_SUBST([BASH_COMPLETIONS_DIR]) >>> +  fi >>> +]) >>> + >>> +AC_DEFUN([LIBVIRT_RESULT_BASH_COMPLETION],[ >>> +  LIBVIRT_RESULT_LIB([BASH_COMPLETION]) >>> +]) >>> diff --git a/tools/Makefile.am b/tools/Makefile.am >>> index 7513a73ac..34a81e69c 100644 >>> --- a/tools/Makefile.am >>> +++ b/tools/Makefile.am >>> @@ -66,6 +66,7 @@ EXTRA_DIST = \ >>>     libvirt-guests.sysconf \ >>>     virt-login-shell.conf \ >>>     virsh-edit.c \ >>> +    bash-completion/vsh \ >>>     $(PODFILES) \ >>>     $(MANINFILES) \ >>>     $(NULL) >>> @@ -332,9 +333,11 @@ POD2MAN = pod2man -c "Virtualization Support" -r >>> "$(PACKAGE)-$(VERSION)" >>>         < $< > $@-t && \ >>>     mv $@-t $@ >>> >>> -install-data-local: install-init install-systemd install-nss >>> +install-data-local: install-init install-systemd install-nss \ >>> +    install-bash-completion >>> >>> -uninstall-local: uninstall-init uninstall-systemd uninstall-nss >>> +uninstall-local: uninstall-init uninstall-systemd uninstall-nss \ >>> +    uninstall-bash-completion >>> >>> install-sysconfig: >>>     $(MKDIR_P) $(DESTDIR)$(sysconfdir)/sysconfig >>> @@ -420,6 +423,21 @@ libvirt-guests.service: libvirt-guests.service.in >>> $(top_builddir)/config.status >>>         mv $@-t $@ >>> >>> >>> +if WITH_BASH_COMPLETION >>> +install-bash-completion: >>> +    $(MKDIR_P) "$(DESTDIR)$(BASH_COMPLETIONS_DIR)" >>> +    $(INSTALL_SCRIPT) $(srcdir)/bash-completion/vsh \ >>> +        "$(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh" >>> + >>> +uninstall-bash-completion: >>> +    rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh >>> +    rmdir $(DESTDIR)$(BASH_COMPLETIONS_DIR) ||: >>> +else ! WITH_BASH_COMPLETION >>> +install-bash-completion: >>> +uninstall-bash-completion: >>> +endif ! WITH_BASH_COMPLETION >>> + >>> + >>> EXTRA_DIST += \ >>>     wireshark/util/genxdrstub.pl \ >>>     wireshark/util/make-dissector-reg >>> diff --git a/tools/bash-completion/vsh b/tools/bash-completion/vsh >>> new file mode 100644 >>> index 000000000..0c923c8b5 >>> --- /dev/null >>> +++ b/tools/bash-completion/vsh >>> @@ -0,0 +1,73 @@ >>> +# >>> +# virsh & virt-admin completion command >>> +# >>> + >>> +_vsh_complete() >>> +{ >>> +    local words cword c=0 i=0 cur RO URI CMDLINE INPUT A >>> + >>> +    # Here, $COMP_WORDS is an array of words on the bash >>> +    # command line that user wants to complete. However, when >>> +    # parsing command line, the default set of word breaks is >>> +    # applied. This doesn't work for us as it mangles libvirt >>> +    # arguments, e.g. connection URI (with the default set it's >>> +    # split into multiple items within the array). Fortunately, >>> +    # there's a fixup function for the array. >>> +    _get_comp_words_by_ref -n "\"'><=;|&(:" -w words -i cword >>> +    COMP_WORDS=( "${words[@]}" ) >>> +    COMP_CWORD=${cword} >>> +    cur=${COMP_WORDS[$COMP_CWORD]} >>> + >>> +    # See what URI is user trying to connect to and if they are >>> +    # connecting RO. Honour that. >>> +    while [ $c -le $COMP_CWORD ]; do >>> +        word="${COMP_WORDS[c]}" >>> +        case "$word" in >>> +            -r|--readonly) RO=1 ;; >>> +            -c|--connect) c=$((++c)); URI=${COMP_WORDS[c]} ;; >>> +            *) if [ $c -ne 0 ] && [ $i -eq 0 ]; then i=$c; break; >>> fi ;; >>> +        esac >>> +        c=$((++c)) >>> +    done >>> + >>> +    CMDLINE= >>> +    if [ -n "${RO}" ]; then >>> +        CMDLINE="${CMDLINE} -r" >>> +    fi >>> +    if [ -n "${URI}" ]; then >>> +        CMDLINE="${CMDLINE} -c ${URI}" >>> +    fi >>> + >> >> When I see all the things you have to do here, wouldn't it be easier to >> have a >> virsh 'option' rather than a 'command' so that we don't have to parse >> anything >> twice and just circumvent the command execution in virsh itself? > > Not really. That would mean parsing the command line in cmdComplete. > Which again might be incomplete and thus yet more code would be needed. > I don't really see a problem with this approach - now that the bash > script is written. > No, there would be no cmdComplete.  And we already parse the whole thing anyway.

>>   You >> would just >> run the same command with '-C' (for example) appended after the program >> name. > > Yeah, there are dozen of other approaches. I've chosen this one. I'm > failing to see why one is better than another one. > Simplicity.

I'm not against this approach, I just wanted an open discussion about an idea.