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

Martin Kletzander <mkletzan@xxxxxxxxxx> · Wed, 8 Nov 2017 16:18:02 +0100

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.

  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.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc

Description: Digital signature
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



