Re: [virt-viewer][PATCH] Introduce bash completion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Michal,

Take this review (and, spoiler alert, ack) with a grain of salt as
I've been out of virt-viewer development for several years now.

On Tue, Mar 26, 2019 at 4:28 PM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
>
> With this change one can get list of domains on the command line:
>
>   $ virt-viewer -c qemu:///system <TAB><TAB>
>   dom1   dom2   ... domN
>
> The list of domains is fetched using virsh, hence the dependency
> on libvirt-client recorded in the spec file. I think it's fair
> to assume that Linux hosts with virt-viewer will have virsh
> available too. If they don't, nothing breaks and no error message
> is printed.
>
> The completer script is inspired by libvirt.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  Makefile.am                 |  2 +-
>  bash-completion/Makefile.am | 20 +++++++++
>  bash-completion/virt-viewer | 90 +++++++++++++++++++++++++++++++++++++
>  configure.ac                |  9 +++-
>  m4/virt-bash-completion.m4  | 83 ++++++++++++++++++++++++++++++++++
>  virt-viewer.spec.in         | 14 ++++++
>  6 files changed, 216 insertions(+), 2 deletions(-)
>  create mode 100644 bash-completion/Makefile.am
>  create mode 100644 bash-completion/virt-viewer
>  create mode 100644 m4/virt-bash-completion.m4
>
> diff --git a/Makefile.am b/Makefile.am
> index 4cfdd59..9dda6a3 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2,7 +2,7 @@ NULL =
>
>  ACLOCAL_AMFLAGS = -I m4
>
> -SUBDIRS = icons src man po data tests
> +SUBDIRS = bash-completion icons src man po data tests
>
>  AM_DISTCHECK_CONFIGURE_FLAGS = --disable-update-mimedb
>  EXTRA_DIST =                                   \
> diff --git a/bash-completion/Makefile.am b/bash-completion/Makefile.am
> new file mode 100644
> index 0000000..be1fc3f
> --- /dev/null
> +++ b/bash-completion/Makefile.am
> @@ -0,0 +1,20 @@
> +EXTRA_DIST = \
> +       $(PACKAGE)
> +
> +install-data-local: install-bash-completion
> +
> +uninstall-local: uninstall-bash-completion
> +
> +if WITH_BASH_COMPLETION
> +install-bash-completion:
> +       $(MKDIR_P) "$(DESTDIR)$(BASH_COMPLETIONS_DIR)"
> +       $(INSTALL_SCRIPT) $(srcdir)/$(PACKAGE) \
> +               "$(DESTDIR)$(BASH_COMPLETIONS_DIR)/$(PACKAGE)"
> +
> +uninstall-bash-completion:
> +       rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/$(PACKAGE)
> +       rmdir $(DESTDIR)$(BASH_COMPLETIONS_DIR) ||:
> +else ! WITH_BASH_COMPLETION
> +install-bash-completion:
> +uninstall-bash-completion:
> +endif ! WITH_BASH_COMPLETION
> diff --git a/bash-completion/virt-viewer b/bash-completion/virt-viewer
> new file mode 100644
> index 0000000..e4a7544
> --- /dev/null
> +++ b/bash-completion/virt-viewer
> @@ -0,0 +1,90 @@
> +#
> +# virt-viewer completer
> +#
> +
> +_virt_viewer_complete()
> +{
> +    local words cword c w cur URI CMDLINE MODE DOMS
> +
> +    # 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 virt-viewer
> +    # 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]}
> +
> +    MODE="--name"
> +    # See what URI is user trying to connect to. Honour that.
> +    for ((c=1; c<=${COMP_CWORD}; c++)); do
> +        case "${COMP_WORDS[c]}" in
> +            -c|--connect)
> +                if [[ -n "${COMP_WORDS[c+1]}" ]]; then
> +                    URI="${COMP_WORDS[c+1]}"
> +                    c=$((++c))
> +                fi
> +                ;;
> +
> +            --connect=*)
> +                w=${COMP_WORDS[c]#*=}
> +                if [[ -z "$w" ]] ; then
> +                    return
> +                fi
> +                URI=$w
> +                ;;
> +
> +            --domain-name)
> +                # Generate list of domain names which is done below
> +                MODE="--name"
> +                ;;
> +
> +            --uuid)
> +                # Generate list of domain names which is done below
> +                MODE="--uuid"
> +                ;;
> +
> +            --id)
> +                # TODO virsh doesn't support listing just IDs yet
> +                ;;
> +        esac
> +    done
> +
> +    case "$cur" in
> +        --connect=*)
> +            # Nada
> +            return
> +            ;;
> +
> +        --kiosk-quit=*)
> +            cur=${cur#*=}
> +            COMPREPLY=($(compgen -W 'never on-disconnect' -- "$cur"))
> +            return
> +            ;;
> +
> +        -*)
> +            COMPREPLY=( $( compgen -W '$( _parse_help "$1" -h )' -- "$cur" ) )
> +            [[ $COMPREPLY == *= ]] && compopt -o nospace

Using "-o" instead of || breaks the syntax check
```
./bash-completion/virt-viewer:70:            [[ $COMPREPLY == *= ]] &&
compopt -o nospace
maint.mk: use "test C1 && test C2", not "test C1 -a C2"; use "test C1
|| test C2", not "test C1 -o C2"
make: *** [../maint.mk:944: sc_prohibit_test_minus_ao] Error 1
exec 3>&-
test "$st" = 0
```

> +            return
> +            ;;
> +    esac
> +
> +    CMDLINE=
> +    if [ -n "${URI}" ]; then
> +        CMDLINE="${CMDLINE} -c ${URI}"
> +    fi
> +
> +    DOMS=($(virsh -q -r ${CMDLINE} list --all ${MODE} 2>/dev/null))
> +
> +    COMPREPLY=($(compgen -W "${DOMS[*]%--}" -- ${cur}))
> +
> +    __ltrim_colon_completions "${cur}"
> +    return
> +} &&
> +complete -F _virt_viewer_complete virt-viewer
> +
> +# vim: ft=sh:et:ts=4:sw=4:tw=80
> +

This extra line in the end of the breaks the syntax check
```
prohibit_empty_lines_at_EOF
../bash-completion/virt-viewer
maint.mk: empty line(s) or no newline at EOF
make: *** [../maint.mk:876: sc_prohibit_empty_lines_at_EOF] Error 1
exec 3>&-
test "$st" = 0
```
> diff --git a/configure.ac b/configure.ac
> index 7b61821..b39c41e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1,4 +1,3 @@
> -
>  AC_INIT([virt-viewer],[9.0])
>  AC_CONFIG_SRCDIR(src/virt-viewer-main.c)
>  AC_CONFIG_MACRO_DIR([m4])
> @@ -12,6 +11,8 @@ AC_CANONICAL_HOST
>  m4_ifndef([AM_SILENT_RULES], [m4_define([AM_SILENT_RULES],[])])
>  AM_SILENT_RULES([yes])
>
> +BASH_COMPLETION_REQUIRED="2.0"
> +
>  # Keep these two definitions in agreement.
>  GLIB2_REQUIRED="2.40"
>  GLIB2_ENCODED_VERSION="GLIB_VERSION_2_40"
> @@ -29,6 +30,7 @@ SPICE_PROTOCOL_REQUIRED="0.12.7"
>  GOVIRT_REQUIRED="0.3.3"
>  REST_REQUIRED="0.8"
>
> +AC_SUBST([BASH_COMPLETION_REQUIRED])
>  AC_SUBST([GLIB2_REQUIRED])
>  AC_SUBST([LIBXML2_REQUIRED])
>  AC_SUBST([LIBVIRT_REQUIRED])
> @@ -99,6 +101,9 @@ AC_DEFINE_UNQUOTED([GETTEXT_PACKAGE],"$GETTEXT_PACKAGE", [GETTEXT package name])
>  VIRT_VIEWER_ARG_NLS
>  VIRT_VIEWER_CHECK_NLS
>
> +VIRT_VIEWER_ARG_BASH_COMPLETION
> +VIRT_VIEWER_CHECK_BASH_COMPLETION
> +
>  PKG_PROG_PKG_CONFIG
>  GLIB_MKENUMS=`$PKG_CONFIG --variable=glib_mkenums glib-2.0`
>  AC_SUBST(GLIB_MKENUMS)
> @@ -250,6 +255,7 @@ AM_CONDITIONAL(ENABLE_UPDATE_MIMEDB, test x$enable_update_mimedb = xyes)
>
>  AC_CONFIG_FILES([
>      Makefile
> +    bash-completion/Makefile
>      data/Makefile
>      data/virt-viewer.wxs
>      icons/Makefile
> @@ -291,3 +297,4 @@ AC_MSG_NOTICE([     LIBVIRT: $LIBVIRT_CFLAGS $LIBVIRT_LIBS])
>  AC_MSG_NOTICE([])
>  AC_MSG_NOTICE([       OVIRT: $OVIRT_CFLAGS $OVIRT_LIBS])
>  AC_MSG_NOTICE([])
> +VIRT_VIEWER_RESULT_BASH_COMPLETION
> diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
> new file mode 100644
> index 0000000..6a7267c
> --- /dev/null
> +++ b/m4/virt-bash-completion.m4
> @@ -0,0 +1,83 @@
> +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 libvirt code.
> +dnl
> +
> +AC_DEFUN([VIRT_VIEWER_ARG_BASH_COMPLETION],[
> +  m4_divert_text([DEFAULTS], [[enable_bash_completion=check]])
> +  AC_ARG_ENABLE([bash_completion],
> +                [AS_HELP_STRING([--enable-bash-completion],
> +                                [bash completion @<:@default=check@:>@])])
> +  m4_divert_text([DEFAULTS], [[with_bash_completions_dir=check]])
> +  AC_ARG_WITH([bash_completions_dir],
> +              [AS_HELP_STRING([--with-bash-completions-dir=DIR],
> +                              [directory containing bash completions scripts @<:@default=check@:>@])])
> +])
> +
> +
> +AC_DEFUN([VIRT_VIEWER_CHECK_BASH_COMPLETION], [
> +  if test "x$enable_bash_completion" != "xno" ; then
> +    PKG_CHECK_MODULES(BASH_COMPLETION, bash-completion >= $BASH_COMPLETION_REQUIRED,
> +                      [with_bash_completion=yes],
> +                      [with_bash_completion=no])
> +    if test "x$enable_bash_completion:x$with_bash_completion" = "yes:no"; then
> +      m4_default(fail_action,
> +        [AC_MSG_ERROR([You must install the ]pc_name[ >= ]pc_version[ pkg-config module to compile virt-viewer])])
> +    fi
> +    enable_bash_completion=$with_bash_completion
> +  fi
> +
> +  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])
> +      with_bash_completions_dir=$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])
> +    else
> +      BASH_COMPLETIONS_DIR=$with_bash_completions_dir
> +    fi
> +    AC_SUBST([BASH_COMPLETIONS_DIR])
> +  fi
> +
> +  AM_CONDITIONAL([WITH_BASH_COMPLETION], [test "x$enable_bash_completion" = "xyes"])
> +])
> +
> +
> +AC_DEFUN([VIRT_VIEWER_RESULT_BASH_COMPLETION],[
> +  if test "x$enable_bash_completion" = "xyes" ; then
> +    AC_MSG_NOTICE([bash completion: $enable_bash_completion DIR: [$with_bash_completions_dir]])
> +  else
> +    AC_MSG_NOTICE([bash completion: $enable_bash_completion])
> +  fi
> +  AC_MSG_NOTICE([])
> +])
> diff --git a/virt-viewer.spec.in b/virt-viewer.spec.in
> index eea5a74..56a3a8a 100644
> --- a/virt-viewer.spec.in
> +++ b/virt-viewer.spec.in
> @@ -11,8 +11,10 @@
>  %endif
>
>  %define with_govirt 0
> +%define with_bash_completion 0
>  %if 0%{?fedora} > 19 || 0%{?rhel} >= 7
>  %define with_govirt 1
> +%define with_bash_completion 1
>  %endif
>
>  Name: @PACKAGE@
> @@ -28,6 +30,12 @@ Requires(postun): %{_sbindir}/update-alternatives
>  Requires(post): desktop-file-utils
>  Requires(postun): desktop-file-utils
>
> +%if %{with_bash_completion}
> +# Our bash completion script uses virsh to list domains
> +Requires: libvirt-client
> +%endif
> +
> +
>  %if 0%{?enable_autotools}
>  BuildRequires: autoconf
>  BuildRequires: automake
> @@ -51,6 +59,9 @@ BuildRequires: gettext
>  %if %{with_govirt}
>  BuildRequires: pkgconfig(govirt-1.0) >= @GOVIRT_REQUIRED@
>  %endif
> +%if %{with_bash_completion}
> +BuildRequires: pkgconfig(bash-completion) >= @BASH_COMPLETION_REQUIRED@
> +%endif
>
>  %if 0%{?fedora} >= 20
>  Obsoletes: spice-client < 0.12.3-2
> @@ -102,5 +113,8 @@ rm -rf $RPM_BUILD_ROOT
>  %{_datadir}/mime/packages/virt-viewer-mime.xml
>  %{_mandir}/man1/virt-viewer.1*
>  %{_mandir}/man1/remote-viewer.1*
> +%if %{with_bash_completion}
> +%{_datadir}/bash-completion/completions/virt-viewer
> +%endif
>
>  %changelog
> --
> 2.19.2
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list

Apart from the two issues pointed out, the patch works as expected!

Here's a diff of the changes that have to be squashed:
```
fidencio@laerte ~/src/upstream/virt-viewer $ git diff
diff --git a/bash-completion/virt-viewer b/bash-completion/virt-viewer
index e4a7544..ad60d46 100644
--- a/bash-completion/virt-viewer
+++ b/bash-completion/virt-viewer
@@ -67,7 +67,7 @@ _virt_viewer_complete()

         -*)
             COMPREPLY=( $( compgen -W '$( _parse_help "$1" -h )' -- "$cur" ) )
-            [[ $COMPREPLY == *= ]] && compopt -o nospace
+            [[ $COMPREPLY == *= ]] && compopt || nospace
             return
             ;;
     esac
@@ -87,4 +87,3 @@ _virt_viewer_complete()
 complete -F _virt_viewer_complete virt-viewer

 # vim: ft=sh:et:ts=4:sw=4:tw=80
-
```

Reviewed-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>

_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list




[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux