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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
