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

Wed Nov 8 15:24:30 UTC 2017

On 11/08/2017 04:18 PM, Martin Kletzander wrote:
> 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 at redhat.com>
>>>> ---
>>>> 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.

How would it generate the list then? -C would need set some boolean flag
so that after parsing the vshReadlineCompletion() is called. So instead
of having the code in a separate cmd*() function it would be worked into
argv parsing. I don't really see the benefit.

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

As I write above, while the bash script would be simpler, vsh's argv
parsing would be more complicated. Or am I missing something?

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

Sure. I'm not saying that my approach is the best there is. It's just
that I understand this solution the most. Just take a look at our argv
parsing - I don't wan to touch that code with 10 feet pole unless I
really need to.

Michal