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

[Eric Blake]

On 11/10/2017 05:01 AM, Michal Privoznik wrote:

> 
> So while implementing this I realized it will not fly. At ARGV level it
> is impossible to tell apart "--domain" and "--domain ".

But it is not impossible to tell apart '"--domain"' (one arg) and
'"--domain" ""' (two args).  If you have trailing whitespace, then tack
on an empty argument.

> But the
> difference is important, because in the first case we want to offer list
> of all --options, while in the second case we want to call the --domain
> completer. 

Yes, the difference between one argument and two is significant - the
completion should always be done on the last argument, but the last
argument needs to be the empty string if it is not in the middle of a word.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

On 11/08/2017 05:26 PM, Eric Blake wrote:
> On 11/08/2017 10:06 AM, Michal Privoznik wrote:
> 
>>> $ virsh complete some-command --arg partial
>>
>> This is excatly what my patches are doing. With one tiny difference. In
>> fact bash script calls:
>>
>>   virsh complete "some-command --arg partial"
>>
>> so that I can have cmdComplete which takes exactly one string argument.
> 
> Ouch. That difference matters, and I don't think you want to do that.
> 
> We don't want to reimplement shell parsing; if the user does:
> 
> virsh snapshot-create-as 'domain with space' 'name with space'
> 'description with space', then my approach feeds those three arguments
> as is (the virsh parser doesn't have to undo any shell quoting), while
> your approach HAS to over-quote the original command line, then
> reimplement shell quoting to remove those quotes in virsh, in order to
> reconstruct the original line in spite of being temporarily squashed
> through one argument.
> 
>> Parsing of the string is then done within cmdComplete. I don't think
>> that we have variable args in virsh for commands, do we?
> 
> Yes, we do.  See 'virsh echo' for an example of its use.

So while implementing this I realized it will not fly. At ARGV level it
is impossible to tell apart "--domain" and "--domain ". But the
difference is important, because in the first case we want to offer list
of all --options, while in the second case we want to call the --domain
completer. Okay, this might not be the best example because there's no
other option that shares the prefix, but for instance 'migrate' command
has:

--timeout
--timeout-suspend
--timeout-postcopy

So if user types in:

  "virsh migrate --timeout"

we have to bring up list:

"--timeout"
"--timeout-suspend"
"--timeout-postcopy"

and only after they type:

  "virsh migrate --timeout "

we can call --timeout completer. Since it's impossible to differentiate
these cases with VSH_OT_ARGV, I'm sticking with what I have now. Sure,
it might has its own problems but those can be solved later.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

On 11/08/2017 05:26 PM, Eric Blake wrote:
> On 11/08/2017 10:06 AM, Michal Privoznik wrote:
> 
>>> $ virsh complete some-command --arg partial
>>
>> This is excatly what my patches are doing. With one tiny difference. In
>> fact bash script calls:
>>
>>   virsh complete "some-command --arg partial"
>>
>> so that I can have cmdComplete which takes exactly one string argument.
> 
> Ouch. That difference matters, and I don't think you want to do that.
> 
> We don't want to reimplement shell parsing; if the user does:
> 
> virsh snapshot-create-as 'domain with space' 'name with space'
> 'description with space', then my approach feeds those three arguments
> as is (the virsh parser doesn't have to undo any shell quoting), while
> your approach HAS to over-quote the original command line, then
> reimplement shell quoting to remove those quotes in virsh, in order to
> reconstruct the original line in spite of being temporarily squashed
> through one argument.

Right. On the other hand, who uses space in file names? ;-)

> 
>> Parsing of the string is then done within cmdComplete. I don't think
>> that we have variable args in virsh for commands, do we?
> 
> Yes, we do.  See 'virsh echo' for an example of its use.

Okay, I will look into this.

> 
>>
>> Correct. And again, my patches do this. For instance:
>>
>>   virsh -r -c qemu+ssh://host/system domifaddr --domain
>>
>> becomes:
>>
>>   virsh -r -c qemu+ssh://host/system complete "domifaddr --domain"
>>
>> And it's the 'complete' command that is responsible for bringing up a
>> list of possible strings.
> 
> Ah, you are trying to put 'complete' after virsh-specific options (-r,
> -c), but before the command (domifaddr).

Yes.

> 
> And if properly implemented, you should be able to have:
> 
> virsh domif
> 
> show 'domifaddr' (and any other commands starting with 'domif') (by
> calling "virsh complete -- domif"),

Yup. This works.

> 
> virsh domifaddr 
> 
> show both a list of domain names AND a list of --options accepted by
> domifaddr (by calling "virsh complete -- domifaddr ''"),

This is still WIP. As I mention in the cover letter, the parser is hard
to grasp for me and therefore this is yet to be implemented. Currently
all you get is list of --options. However, completers for --option work.
For instance:

virsh start --domain 

brings up list of domains to start.

> 
> virsh domifaddr --
> 
> show a list of long options for domifaddr (by calling "virsh complete --
> domifaddr --") (yes, I'm specifically making sure the second '--'
> doesn't get eaten by getopts, by inserting 'complete --' rather than
> just 'complete'), and
> 
> virsh domifaddr a
> 
> show a filtered list of just domain names starting with 'a' (by calling
> "virsh complete domifaddr a").
> 
> That is, the completions should be context-sensitive based on how much
> of the command line is present prior to the , and should NOT
> need the user to type an explicit --domain just to get domain completions.
> 

Yeah, that's the idea. And I'm looking into the code, but it's not that
easy for me to understand it. So, if anybody wants to help, be my guest.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Eric Blake]

On 11/08/2017 10:06 AM, Michal Privoznik wrote:

>> $ virsh complete some-command --arg partial
> 
> This is excatly what my patches are doing. With one tiny difference. In
> fact bash script calls:
> 
>   virsh complete "some-command --arg partial"
> 
> so that I can have cmdComplete which takes exactly one string argument.

Ouch. That difference matters, and I don't think you want to do that.

We don't want to reimplement shell parsing; if the user does:

virsh snapshot-create-as 'domain with space' 'name with space'
'description with space', then my approach feeds those three arguments
as is (the virsh parser doesn't have to undo any shell quoting), while
your approach HAS to over-quote the original command line, then
reimplement shell quoting to remove those quotes in virsh, in order to
reconstruct the original line in spite of being temporarily squashed
through one argument.

> Parsing of the string is then done within cmdComplete. I don't think
> that we have variable args in virsh for commands, do we?

Yes, we do.  See 'virsh echo' for an example of its use.

> 
> Correct. And again, my patches do this. For instance:
> 
>   virsh -r -c qemu+ssh://host/system domifaddr --domain
> 
> becomes:
> 
>   virsh -r -c qemu+ssh://host/system complete "domifaddr --domain"
> 
> And it's the 'complete' command that is responsible for bringing up a
> list of possible strings.

Ah, you are trying to put 'complete' after virsh-specific options (-r,
-c), but before the command (domifaddr).

And if properly implemented, you should be able to have:

virsh domif

show 'domifaddr' (and any other commands starting with 'domif') (by
calling "virsh complete -- domif"),

virsh domifaddr 

show both a list of domain names AND a list of --options accepted by
domifaddr (by calling "virsh complete -- domifaddr ''"),

virsh domifaddr --

show a list of long options for domifaddr (by calling "virsh complete --
domifaddr --") (yes, I'm specifically making sure the second '--'
doesn't get eaten by getopts, by inserting 'complete --' rather than
just 'complete'), and

virsh domifaddr a

show a filtered list of just domain names starting with 'a' (by calling
"virsh complete domifaddr a").

That is, the completions should be context-sensitive based on how much
of the command line is present prior to the , and should NOT
need the user to type an explicit --domain just to get domain completions.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

On 11/08/2017 04:53 PM, Eric Blake wrote:
> On 11/08/2017 09:09 AM, 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 
> 
>>>
>>> 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.
>>
>>>   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.
>>
> 
> [I haven't read this thread closely yet, just adding a drive-by comment]
> 
> Several years ago, when autocompletion was attempted (and failed) as a
> GSoC project, I had several ideas on how completion should work, and how
> it should be tested.
> 
> Ideally, we need a new virsh command 'complete', which takes varargs,
> and then performs completion based on those args.  So the bash script
> for completion for this scenario:
> 
> $ virsh some-command --arg partial
> 
> is as simple as having the completion function in bash call:
> 
> $ virsh complete some-command --arg partial

This is excatly what my patches are doing. With one tiny difference. In
fact bash script calls:

  virsh complete "some-command --arg partial"

so that I can have cmdComplete which takes exactly one string argument.
Parsing of the string is then done within cmdComplete. I don't think
that we have variable args in virsh for commands, do we?

> 
> The complete command in virsh should then know that it is performing
> completion on some-command, and do enough arg parsing to determine how
> to complete 'partial' in the current context of whatever argument
> some-command would be parsing at that point.
> 
> If a user in bash backs up the cursor and types  somewhere other
> than the end of the line, hopefully bash gives us enough hooks to call
> 'virsh complete args-up-to-TAB' by merely truncating whatever appears
> beyond the point where the user attempted tab completion.
> 
> ALL the completion logic lives in virsh, nothing in bash - all bash has
> to do is insert 'complete' and call into virsh.  That is, once you've
> written the bash script, it should NEVER need future modification,
> because any further improvements to completion will live in virsh
> (whether you use virsh in interactive mode or in batch mode from the shell).

Correct. And again, my patches do this. For instance:

  virsh -r -c qemu+ssh://host/system domifaddr --domain

becomes:

  virsh -r -c qemu+ssh://host/system complete "domifaddr --domain"

And it's the 'complete' command that is responsible for bringing up a
list of possible strings.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Eric Blake]

On 11/08/2017 09:09 AM, 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 

>>
>> 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.
> 
>>   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.
> 

[I haven't read this thread closely yet, just adding a drive-by comment]

Several years ago, when autocompletion was attempted (and failed) as a
GSoC project, I had several ideas on how completion should work, and how
it should be tested.

Ideally, we need a new virsh command 'complete', which takes varargs,
and then performs completion based on those args.  So the bash script
for completion for this scenario:

$ virsh some-command --arg partial

is as simple as having the completion function in bash call:

$ virsh complete some-command --arg partial

The complete command in virsh should then know that it is performing
completion on some-command, and do enough arg parsing to determine how
to complete 'partial' in the current context of whatever argument
some-command would be parsing at that point.

If a user in bash backs up the cursor and types  somewhere other
than the end of the line, hopefully bash gives us enough hooks to call
'virsh complete args-up-to-TAB' by merely truncating whatever appears
beyond the point where the user attempted tab completion.

ALL the completion logic lives in virsh, nothing in bash - all bash has
to do is insert 'complete' and call into virsh.  That is, once you've
written the bash script, it should NEVER need future modification,
because any further improvements to completion will live in virsh
(whether you use virsh in interactive mode or in batch mode from the shell).

I don't know how much my original idea has carried over into your
proposal in this thread, but you may want to read the emails I posted
from the archives on the topic, for more ideas.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

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 
 ---
 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 0..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 .
 +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
 +    

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

[Martin Kletzander]

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 
---
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 0..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 .
+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 

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

[Michal Privoznik]

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 
>> ---
>> 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 0..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 .
>> +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 

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

[Martin Kletzander]

On Tue, Nov 07, 2017 at 01:22:56PM +0100, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
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 0..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 .
+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] [PATCH 08/11] tools: Provide bash autompletion file

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 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 0..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 .
+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
+  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])
+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