8:22 a.m.
Due to some naming convention the bash script for inactive
virsh/virt-admin wasn't loaded. Fix this.
At the same time, fix two small nits raised in the other patch set for
bash completion.
Michal Privoznik (3):
virshDomainNameCompleter: Prune accepted flags
virsh: Offer only persistent domains for autostart
tools: Make symlinks to vsh bash-completion script
libvirt.spec.in | 24 +++++++++++++++++++++++-
tools/Makefile.am | 7 ++++++-
tools/virsh-completer.c | 13 +++----------
tools/virsh-domain.c | 2 +-
4 files changed, 33 insertions(+), 13 deletions(-)
--
2.13.6
8:22 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
actually used in virsh code. Remove the unused ones.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-completer.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index e216d9076..e3b8234b4 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -45,18 +45,11 @@ virshDomainNameCompleter(vshControl *ctl,
virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+ VIR_CONNECT_LIST_DOMAINS_OTHER |
+ VIR_CONNECT_LIST_DOMAINS_PAUSED |
VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
- VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
VIR_CONNECT_LIST_DOMAINS_RUNNING |
- VIR_CONNECT_LIST_DOMAINS_PAUSED |
- VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
- VIR_CONNECT_LIST_DOMAINS_OTHER |
- VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
- VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
- VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
- VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
- VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
- VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+ VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
NULL);
if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
--
2.13.6
5:40 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
On Thu, Jan 25, 2018 at 03:22:18PM +0100, Michal Privoznik wrote:
Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
actually used in virsh code. Remove the unused ones.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-completer.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
ACK
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index e216d9076..e3b8234b4 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -45,18 +45,11 @@ virshDomainNameCompleter(vshControl *ctl,
virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+ VIR_CONNECT_LIST_DOMAINS_OTHER |
+ VIR_CONNECT_LIST_DOMAINS_PAUSED |
VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
- VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
VIR_CONNECT_LIST_DOMAINS_RUNNING |
- VIR_CONNECT_LIST_DOMAINS_PAUSED |
- VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
- VIR_CONNECT_LIST_DOMAINS_OTHER |
- VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
- VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
- VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
- VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
- VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
- VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+ VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
NULL);
if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:09 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
actually used in virsh code. Remove the unused ones.
I don't think this is well justified. All of the flags are implemented
in virsh code in general.
I see almost all of them implemented in the cmdList command.
You either need to rewrite the commit message or the patch below is
flawed.
7:34 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
On 01/31/2018 01:09 PM, Peter Krempa wrote:
On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
> actually used in virsh code. Remove the unused ones.
I don't think this is well justified. All of the flags are implemented
in virsh code in general.
I see almost all of them implemented in the cmdList command.
We don't have to care about cmdList since it does not use
virshDomainNameCompleter. What this patch addresses is use of
VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
commands accept domains only from a small subset of those. It's safe to
say that 99% of commands take active/inactive domains, and the rest 1%
is more specific. Anyway, lets try to print out all used flags for this
completer:
libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
cut -d')' -f 1 | sort -u
VIR_CONNECT_LIST_DOMAINS_ACTIVE
VIR_CONNECT_LIST_DOMAINS_INACTIVE
VIR_CONNECT_LIST_DOMAINS_OTHER
VIR_CONNECT_LIST_DOMAINS_PAUSED
VIR_CONNECT_LIST_DOMAINS_PERSISTENT
VIR_CONNECT_LIST_DOMAINS_RUNNING
You either need to rewrite the commit message or the patch below is
flawed.
I guess you just misunderstood this patch.
Michal
7:39 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
On Wed, Jan 31, 2018 at 14:34:24 +0100, Michal Privoznik wrote:
On 01/31/2018 01:09 PM, Peter Krempa wrote:
> On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
>> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>> actually used in virsh code. Remove the unused ones.
>
> I don't think this is well justified. All of the flags are implemented
> in virsh code in general.
>
> I see almost all of them implemented in the cmdList command.
We don't have to care about cmdList since it does not use
virshDomainNameCompleter. What this patch addresses is use of
VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
commands accept domains only from a small subset of those. It's safe to
say that 99% of commands take active/inactive domains, and the rest 1%
is more specific. Anyway, lets try to print out all used flags for this
completer:
All this information is not present in the commit message. You
generalize that the "flags are (not) actually used in virsh code", which
is simply false. The commit message does in no way mention that you are
specifically talking about usage of the flags in the completion code,
which leads to false assumptions.
libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
cut -d')' -f 1 | sort -u
VIR_CONNECT_LIST_DOMAINS_ACTIVE
VIR_CONNECT_LIST_DOMAINS_INACTIVE
VIR_CONNECT_LIST_DOMAINS_OTHER
VIR_CONNECT_LIST_DOMAINS_PAUSED
VIR_CONNECT_LIST_DOMAINS_PERSISTENT
VIR_CONNECT_LIST_DOMAINS_RUNNING
>
> You either need to rewrite the commit message or the patch below is
> flawed.
I guess you just misunderstood this patch.
You did not explain it well enough in the commit message.
8:17 a.m.
New subject: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags
On 01/31/2018 02:39 PM, Peter Krempa wrote:
On Wed, Jan 31, 2018 at 14:34:24 +0100, Michal Privoznik wrote:
> On 01/31/2018 01:09 PM, Peter Krempa wrote:
>> On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
>>> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>>> actually used in virsh code. Remove the unused ones.
>>
>> I don't think this is well justified. All of the flags are implemented
>> in virsh code in general.
>>
>> I see almost all of them implemented in the cmdList command.
>
> We don't have to care about cmdList since it does not use
> virshDomainNameCompleter. What this patch addresses is use of
> VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
> commands accept domains only from a small subset of those. It's safe to
> say that 99% of commands take active/inactive domains, and the rest 1%
> is more specific. Anyway, lets try to print out all used flags for this
> completer:
All this information is not present in the commit message. You
generalize that the "flags are (not) actually used in virsh code", which
is simply false. The commit message does in no way mention that you are
specifically talking about usage of the flags in the completion code,
which leads to false assumptions.
Ah sorry, I though that "virsh*Completer: " prefix was clear enough. It
isn't. Okay, I'll update the commit message before pushing.
>
> libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
> cut -d')' -f 1 | sort -u
>
> VIR_CONNECT_LIST_DOMAINS_ACTIVE
> VIR_CONNECT_LIST_DOMAINS_INACTIVE
> VIR_CONNECT_LIST_DOMAINS_OTHER
> VIR_CONNECT_LIST_DOMAINS_PAUSED
> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
> VIR_CONNECT_LIST_DOMAINS_RUNNING
>
>>
>> You either need to rewrite the commit message or the patch below is
>> flawed.
>
> I guess you just misunderstood this patch.
You did not explain it well enough in the commit message.
Michal
8:22 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: Offer only persistent domains for autostart
The 'autostart' command accepts only persistent domains. Make the
completer return only those.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0f329d6d7..5a0e0c1b2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1037,7 +1037,7 @@ static const vshCmdInfo info_autostart[] = {
};
static const vshCmdOptDef opts_autostart[] = {
- VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+ VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_PERSISTENT),
{.name = "disable",
.type = VSH_OT_BOOL,
.help = N_("disable autostarting")
--
2.13.6
5:40 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: Offer only persistent domains for autostart
On Thu, Jan 25, 2018 at 03:22:19PM +0100, Michal Privoznik wrote:
The 'autostart' command accepts only persistent domains. Make
the
completer return only those.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0f329d6d7..5a0e0c1b2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1037,7 +1037,7 @@ static const vshCmdInfo info_autostart[] = {
};
static const vshCmdOptDef opts_autostart[] = {
- VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+ VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_PERSISTENT),
{.name = "disable",
.type = VSH_OT_BOOL,
.help = N_("disable autostarting")
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:22 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Make symlinks to vsh bash-completion script
The bash-completion project documents that only those scripts
from $BASH_COMPLETIONS_DIR that share name with the current
command for which <TAB> was hit are loaded [1]. This means, that
vsh script we have there is not loaded. We have to create
symlinks for virsh and virt-admin.
At the same time, we have to create new RPM package because
virt-admin and client packages are independent. That means we
cannot place the vsh script in either of them. What we can do is
to have a different package that contains the completion script
and then virt-admin and client packages contain only the symlink
and require the bash-completion package.
1: https://github.com/scop/bash-completion#faq
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
libvirt.spec.in | 24 +++++++++++++++++++++++-
tools/Makefile.am | 7 ++++++-
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index f62d7d324..1879e1f8b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1017,6 +1017,9 @@ Requires: gnutls-utils
# Needed for probing the power management features of the host.
Requires: pm-utils
%endif
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
%description client
The client binaries needed to access the virtualization
@@ -1041,10 +1044,22 @@ Summary: Set of tools to control libvirt daemon
Group: Development/Libraries
Requires: %{name}-libs = %{version}-%{release}
Requires: readline
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
%description admin
The client side utilities to control the libvirt daemon.
+%if %{with_bash_completion}
+%package bash-completion
+Summary: Bash completion script
+Group: Development/Libraries
+
+%description bash-completion
+Bash completion script stub.
+%endif
+
%if %{with_wireshark}
%package wireshark
Summary: Wireshark dissector plugin for libvirt RPC transactions
@@ -2059,7 +2074,7 @@ exit 0
%{_datadir}/systemtap/tapset/libvirt_functions.stp
%if %{with_bash_completion}
-%{_datadir}/bash-completion/completions/vsh
+%{_datadir}/bash-completion/completions/virsh
%endif
@@ -2111,7 +2126,14 @@ exit 0
%files admin
%{_mandir}/man1/virt-admin.1*
%{_bindir}/virt-admin
+%if %{with_bash_completion}
+%{_datadir}/bash-completion/completions/virt-admin
+%endif
+%if %{with_bash_completion}
+%files bash-completion
+%{_datadir}/bash-completion/completions/vsh
+%endif
%if %{with_wireshark}
%files wireshark
diff --git a/tools/Makefile.am b/tools/Makefile.am
index e9597cdb4..e173f5634 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -424,9 +424,14 @@ install-bash-completion:
$(MKDIR_P) "$(DESTDIR)$(BASH_COMPLETIONS_DIR)"
$(INSTALL_SCRIPT) $(srcdir)/bash-completion/vsh \
"$(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh"
+ ( cd $(DESTDIR)$(BASH_COMPLETIONS_DIR) && \
+ $(LN_S) vsh virsh && \
+ $(LN_S) vsh virt-admin )
uninstall-bash-completion:
- rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh
+ rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh \
+ $(DESTDIR)$(BASH_COMPLETIONS_DIR)/virsh \
+ $(DESTDIR)$(BASH_COMPLETIONS_DIR)/virt-admin
rmdir $(DESTDIR)$(BASH_COMPLETIONS_DIR) ||:
else ! WITH_BASH_COMPLETION
install-bash-completion:
--
2.13.6
5:42 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Make symlinks to vsh bash-completion script
On Thu, Jan 25, 2018 at 03:22:20PM +0100, Michal Privoznik wrote:
The bash-completion project documents that only those scripts
from $BASH_COMPLETIONS_DIR that share name with the current
command for which <TAB> was hit are loaded [1]. This means, that
vsh script we have there is not loaded. We have to create
symlinks for virsh and virt-admin.
Good to know, I recall that the completions had their own way how to say for
which commands they can complete, but probably my memory doesn't serve me well.
Also it's not ECC, so...
At the same time, we have to create new RPM package because
virt-admin and client packages are independent. That means we
cannot place the vsh script in either of them. What we can do is
to have a different package that contains the completion script
and then virt-admin and client packages contain only the symlink
and require the bash-completion package.
1: https://github.com/scop/bash-completion#faq
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
libvirt.spec.in | 24 +++++++++++++++++++++++-
tools/Makefile.am | 7 ++++++-
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index f62d7d324..1879e1f8b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1017,6 +1017,9 @@ Requires: gnutls-utils
# Needed for probing the power management features of the host.
Requires: pm-utils
%endif
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
%description client
The client binaries needed to access the virtualization
@@ -1041,10 +1044,22 @@ Summary: Set of tools to control libvirt daemon
Group: Development/Libraries
Requires: %{name}-libs = %{version}-%{release}
Requires: readline
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
%description admin
The client side utilities to control the libvirt daemon.
+%if %{with_bash_completion}
+%package bash-completion
+Summary: Bash completion script
I'd say "Bash completion script for libvirt clients"
+Group: Development/Libraries
+
+%description bash-completion
+Bash completion script stub.
Similarly here, add info that it's for libvirt cli or something.
Otherwise looks good, ACK with those two things fixed.
2484
days inactive
2490
days old
10 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa