On Mon, May 6, 2019 at 9:25 AM Fabiano Fidêncio <fidencio@xxxxxxxxxx> 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@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
> ```
>
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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
