[virt-tools-list] [virt-viewer][PATCH] Introduce bash completion

[Fabiano Fidêncio]

On Mon, May 6, 2019 at 9:25 AM Fabiano Fidêncio <fidencio at redhat.com> wrote:
>
> 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 at redhat.com> 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 at redhat.com>
> > ---
> >  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
> ```
>

Hmm. Michal pointed out that `-o` is an argument for compopt. So, we'd
actually have to adapt make syntax check to ignore this one.
So, I'm taking my "R-b" back as Michal will submit a v2 of the patch.

Best Regards,
-- 
Fabiano Fidêncio