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

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.

>  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.

Michal -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list