11:18 a.m.
It started simple - I pointed somebody to debug logs kbase article only
to realize our own examples don't work. And ended up fixing bash
completion. Fans of 'Malcolm in the Middle' know the feeling.
Michal Prívozník (6):
kbase: Use virt-admin daemon-timeout correctly
virt-admin: Make --timeout of daemon-timeout positional argument
bash-completion: Run virsh/virt-admin in quiet mode
vsh: Close stderr among with stdin in cmdComplete
vsh: Restore original rl_line_buffer after completion
vsh: Don't crash when @text is NULL in vshCompleterFilter()
docs/kbase/debuglogs.rst | 4 ++--
docs/manpages/virt-admin.rst | 2 +-
tools/bash-completion/vsh.in | 2 +-
tools/virt-admin.c | 1 +
tools/vsh.c | 16 +++++++++++-----
5 files changed, 16 insertions(+), 9 deletions(-)
--
2.44.1
11:18 a.m.
New subject: [PATCH 1/6] kbase: Use virt-admin daemon-timeout correctly
In a few examples we recommend disabling daemon timeout when
fetching debug logs. While it makes sense the actual syntax used
results in an error:
# virt-admin daemon-timeout 0
error: unexpected data '0'
This is because --timeout is required. Update examples to include
it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/kbase/debuglogs.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst
index f08132d099..dff2cfd144 100644
--- a/docs/kbase/debuglogs.rst
+++ b/docs/kbase/debuglogs.rst
@@ -25,7 +25,7 @@ the system clears this setting::
# virt-admin -c virtqemud:///system daemon-log-outputs "3:journald
1:file:/var/log/libvirt/libvirtd.log"
# virt-admin -c virtqemud:///system daemon-log-filters "3:remote 4:event
3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access
1:*"
- # virt-admin -c virtqemud:///system daemon-timeout 0
+ # virt-admin -c virtqemud:///system daemon-timeout --timeout 0
The last command disabling timeout of the daemon is available since
``libvirt-8.6.0``. With older versions make sure to reproduce the issue within
@@ -224,7 +224,7 @@ is re-started with another command.
To prevent auto-shutdown of the daemon you can use the following command::
- virt-admin daemon-timeout 0
+ virt-admin daemon-timeout --timeout 0
The above is introduced in libvirt-8.6.0.
--
2.44.1
12:44 p.m.
New subject: [PATCH 1/6] kbase: Use virt-admin daemon-timeout correctly
On Mon, May 27, 2024 at 11:18:49 +0200, Michal Privoznik wrote:
In a few examples we recommend disabling daemon timeout when
fetching debug logs. While it makes sense the actual syntax used
results in an error:
# virt-admin daemon-timeout 0
error: unexpected data '0'
This is because --timeout is required. Update examples to include
it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/kbase/debuglogs.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:18 a.m.
New subject: [PATCH 2/6] virt-admin: Make --timeout of daemon-timeout positional argument
We currently require full argument specification:
virt-admin daemon-timeout --timeout X
Well, the '--timeout' feels a bit redundant. Turn the argument
into a positional so that the following works too:
virt-admin daemon-timeout X
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/manpages/virt-admin.rst | 2 +-
tools/virt-admin.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/docs/manpages/virt-admin.rst b/docs/manpages/virt-admin.rst
index 5108781636..54a6512ef7 100644
--- a/docs/manpages/virt-admin.rst
+++ b/docs/manpages/virt-admin.rst
@@ -320,7 +320,7 @@ daemon-timeout
::
- daemon-timeout --timeout NUM
+ daemon-timeout [--timeout] NUM
Sets the daemon timeout to the value of '--timeout' argument. Use ``--timeout
0``
to disable auto-shutdown of the daemon.
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 1805618035..3eb4f0f3fd 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1009,6 +1009,7 @@ static const vshCmdOptDef opts_daemon_timeout[] = {
{.name = "timeout",
.type = VSH_OT_INT,
.required = true,
+ .positional = true,
.help = N_("number of seconds the daemon will run without any active
connection"),
},
{.name = NULL}
--
2.44.1
12:55 p.m.
New subject: [PATCH 2/6] virt-admin: Make --timeout of daemon-timeout
positional argument
On Mon, May 27, 2024 at 11:18:50 +0200, Michal Privoznik wrote:
We currently require full argument specification:
virt-admin daemon-timeout --timeout X
Well, the '--timeout' feels a bit redundant. Turn the argument
into a positional so that the following works too:
virt-admin daemon-timeout X
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/manpages/virt-admin.rst | 2 +-
tools/virt-admin.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
It was a bit of a deliberate decision, because any positional argument
can't be made optional and must be kept in order. Historically also the
parser did weird things for stuff that wasn't really considered
positional but was parsed so regardless.
In this case though ... I can't really see another option being added or
timeout becoming optional especially since '0' is used as way to disable
the timeout.
So your justification here makes sense, it's just that I really don't
like positional arguments.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:23 p.m.
New subject: [PATCH 2/6] virt-admin: Make --timeout of daemon-timeout
positional argument
On 5/27/24 12:55, Peter Krempa wrote:
On Mon, May 27, 2024 at 11:18:50 +0200, Michal Privoznik wrote:
> We currently require full argument specification:
>
> virt-admin daemon-timeout --timeout X
>
> Well, the '--timeout' feels a bit redundant. Turn the argument
> into a positional so that the following works too:
>
> virt-admin daemon-timeout X
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> docs/manpages/virt-admin.rst | 2 +-
> tools/virt-admin.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
It was a bit of a deliberate decision, because any positional argument
can't be made optional and must be kept in order. Historically also the
parser did weird things for stuff that wasn't really considered
positional but was parsed so regardless.
In this case though ... I can't really see another option being added or
timeout becoming optional especially since '0' is used as way to disable
the timeout.
This was exactly my internal reasoning when writing this patch because I
too dislike positional arguments.
So your justification here makes sense, it's just that I really don't
like positional arguments.
Since our docs are fixed by previous patch, this one is just a
'shortcut' and strictly speaking it's not needed. I can drop it, I don't
mind.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
Michal
11:18 a.m.
New subject: [PATCH 3/6] bash-completion: Run virsh/virt-admin in quiet mode
In some cases (e.g. when virt-admin connects to the default URI)
some info message is printed onto stdout (using vshPrintExtra()).
This hurts user experience, just consider:
virt-admin<TAB><TAB>
NOTE\:\ Connecting\ to\ default\ daemon.\ Specify\ daemon\ using\ -c\ \(e.g.\
virtqemud\:///system\)
when no daemon is running. Suppress extra prints by passing '-q'
in the bash-completion script.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/bash-completion/vsh.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/bash-completion/vsh.in b/tools/bash-completion/vsh.in
index 897f74cc08..5692788d63 100644
--- a/tools/bash-completion/vsh.in
+++ b/tools/bash-completion/vsh.in
@@ -30,7 +30,7 @@ _@command@_complete()
c=$((++c))
done
- CMDLINE=( )
+ CMDLINE=( "-q" )
if [ -n "${RO}" ]; then
CMDLINE+=("-r")
fi
--
2.44.1
12:55 p.m.
New subject: [PATCH 3/6] bash-completion: Run virsh/virt-admin in quiet mode
On Mon, May 27, 2024 at 11:18:51 +0200, Michal Privoznik wrote:
In some cases (e.g. when virt-admin connects to the default URI)
some info message is printed onto stdout (using vshPrintExtra()).
This hurts user experience, just consider:
virt-admin<TAB><TAB>
NOTE\:\ Connecting\ to\ default\ daemon.\ Specify\ daemon\ using\ -c\ \(e.g.\
virtqemud\:///system\)
when no daemon is running. Suppress extra prints by passing '-q'
in the bash-completion script.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/bash-completion/vsh.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:18 a.m.
New subject: [PATCH 4/6] vsh: Close stderr among with stdin in cmdComplete
Our completer callbacks must refrain from printing anything onto
stderr, but unfortunately that's not how service code around
behaves. It may call vshError() and what not. Rather trying to
fix all possible paths (just consider opening a connection), just
close the stderr. We're already closing stdin.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 61a3066f49..de869248b4 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3488,17 +3488,20 @@ const vshCmdInfo info_complete = {
#ifdef WITH_READLINE
-static virOnceControl vshCmdCompleteCloseStdinOnce = VIR_ONCE_CONTROL_INITIALIZER;
+static virOnceControl vshCmdCompleteCloseStdinStderrOnce = VIR_ONCE_CONTROL_INITIALIZER;
static void
-vshCmdCompleteCloseStdin(void)
+vshCmdCompleteCloseStdinStderr(void)
{
/* In non-interactive mode which is how the 'complete' command is intended
* to be used we need to ensure that any authentication callback will not
- * attempt to read any input which would break the completion */
+ * attempt to read any input which would break the completion. Similarly,
+ * printing anything onto stderr should be avoided. */
int stdin_fileno = STDIN_FILENO;
+ int stderr_fileno = STDERR_FILENO;
VIR_FORCE_CLOSE(stdin_fileno);
+ VIR_FORCE_CLOSE(stderr_fileno);
}
@@ -3519,7 +3522,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
* need to prevent auth hooks reading any input. Therefore, we
* have to close stdin and then connect ourselves. */
if (!ctl->imode) {
- if (virOnce(&vshCmdCompleteCloseStdinOnce, vshCmdCompleteCloseStdin) < 0)
+ if (virOnce(&vshCmdCompleteCloseStdinStderrOnce,
vshCmdCompleteCloseStdinStderr) < 0)
return false;
}
--
2.44.1
12:56 p.m.
New subject: [PATCH 4/6] vsh: Close stderr among with stdin in cmdComplete
On Mon, May 27, 2024 at 11:18:52 +0200, Michal Privoznik wrote:
Our completer callbacks must refrain from printing anything onto
stderr, but unfortunately that's not how service code around
behaves. It may call vshError() and what not. Rather trying to
fix all possible paths (just consider opening a connection), just
close the stderr. We're already closing stdin.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:18 a.m.
New subject: [PATCH 5/6] vsh: Restore original rl_line_buffer after completion
Problem with readline is its API. It's basically a bunch of
global variables with no clear dependencies between them. In this
specific case that I'm seeing: in interactive mode the
cmdComplete() causes instant crash of virsh/virt-admin:
==27999== Invalid write of size 1
==27999== at 0x516EF71: _rl_init_line_state (readline.c:742)
==27999== by 0x5170054: rl_initialize (readline.c:1192)
==27999== by 0x516E5E4: readline (readline.c:379)
==27999== by 0x1B7024: vshReadline (vsh.c:3048)
==27999== by 0x140DCF: main (virsh.c:905)
==27999== Address 0x0 is not stack'd, malloc'd or (recently) free'd
This is because readline keeps a copy of pointer to
rl_line_buffer and the moment cmdComplete() returns and readline
takes over, it accesses the copy which is now a dangling pointer.
To fix this, just keep the original state of rl_line_buffer and
restore it.
Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/vsh.c b/tools/vsh.c
index de869248b4..c91d756885 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
const vshClientHooks *hooks = ctl->hooks;
const char *lastArg = NULL;
const char **args = NULL;
+ char *old_rl_line_buffer = NULL;
g_auto(GStrv) matches = NULL;
char **iter;
@@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
vshReadlineInit(ctl);
+ old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd, "string"))))
rl_line_buffer = g_strdup("");
@@ -3540,6 +3542,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
matches = vshReadlineCompletion(lastArg, 0, 0);
g_clear_pointer(&rl_line_buffer, g_free);
+ rl_line_buffer = g_steal_pointer(&old_rl_line_buffer);
if (!matches)
return false;
--
2.44.1
2:53 p.m.
New subject: [PATCH 5/6] vsh: Restore original rl_line_buffer after completion
On Mon, May 27, 2024 at 11:18:53 +0200, Michal Privoznik wrote:
Problem with readline is its API. It's basically a bunch of
global variables with no clear dependencies between them. In this
specific case that I'm seeing: in interactive mode the
cmdComplete() causes instant crash of virsh/virt-admin:
==27999== Invalid write of size 1
==27999== at 0x516EF71: _rl_init_line_state (readline.c:742)
==27999== by 0x5170054: rl_initialize (readline.c:1192)
==27999== by 0x516E5E4: readline (readline.c:379)
==27999== by 0x1B7024: vshReadline (vsh.c:3048)
==27999== by 0x140DCF: main (virsh.c:905)
==27999== Address 0x0 is not stack'd, malloc'd or (recently) free'd
This is because readline keeps a copy of pointer to
rl_line_buffer and the moment cmdComplete() returns and readline
takes over, it accesses the copy which is now a dangling pointer.
To fix this, just keep the original state of rl_line_buffer and
restore it.
Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/vsh.c b/tools/vsh.c
index de869248b4..c91d756885 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
const vshClientHooks *hooks = ctl->hooks;
const char *lastArg = NULL;
const char **args = NULL;
+ char *old_rl_line_buffer = NULL;
g_auto(GStrv) matches = NULL;
char **iter;
@@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
vshReadlineInit(ctl);
+ old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd, "string"))))
rl_line_buffer = g_strdup("");
I've wondered about this when I was reworking the parser code. Do we
even need to call 'vshReadlineCompletion' (which just does:
return rl_completion_matches(text, vshReadlineParse);
from 'cmdComplete'? What does that do? Can't we use
'vshReadlineParse'
instead?
The fix makes sense as is, but I never really understood why
'cmdComplete' even needed readline in the first place.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:23 p.m.
New subject: [PATCH 5/6] vsh: Restore original rl_line_buffer after completion
On 5/27/24 14:53, Peter Krempa wrote:
On Mon, May 27, 2024 at 11:18:53 +0200, Michal Privoznik wrote:
> Problem with readline is its API. It's basically a bunch of
> global variables with no clear dependencies between them. In this
> specific case that I'm seeing: in interactive mode the
> cmdComplete() causes instant crash of virsh/virt-admin:
>
> ==27999== Invalid write of size 1
> ==27999== at 0x516EF71: _rl_init_line_state (readline.c:742)
> ==27999== by 0x5170054: rl_initialize (readline.c:1192)
> ==27999== by 0x516E5E4: readline (readline.c:379)
> ==27999== by 0x1B7024: vshReadline (vsh.c:3048)
> ==27999== by 0x140DCF: main (virsh.c:905)
> ==27999== Address 0x0 is not stack'd, malloc'd or (recently) free'd
>
> This is because readline keeps a copy of pointer to
> rl_line_buffer and the moment cmdComplete() returns and readline
> takes over, it accesses the copy which is now a dangling pointer.
>
> To fix this, just keep the original state of rl_line_buffer and
> restore it.
>
> Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
> Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> tools/vsh.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index de869248b4..c91d756885 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
> const vshClientHooks *hooks = ctl->hooks;
> const char *lastArg = NULL;
> const char **args = NULL;
> + char *old_rl_line_buffer = NULL;
> g_auto(GStrv) matches = NULL;
> char **iter;
>
> @@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>
> vshReadlineInit(ctl);
>
> + old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
> if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd,
"string"))))
> rl_line_buffer = g_strdup("");
>
I've wondered about this when I was reworking the parser code. Do we
even need to call 'vshReadlineCompletion' (which just does:
return rl_completion_matches(text, vshReadlineParse);
from 'cmdComplete'? What does that do? Can't we use
'vshReadlineParse'
instead?
We could call rl_completion_matches(), sure. But what we can not do is
call just vshReadlineParse(). Unless we want to mimic what
rl_completion_matches() does:
https://git.savannah.gnu.org/cgit/readline.git/tree/complete.c#n2214
In particular, we would need to call vshReadlineParse() in a loop until
it returns NULL, then reorder returned string list. I just found it
easier to call vshReadlineCompletion() which is what readline would call
on <TAB><TAB> in interactive mode.
Oh, and IIRC there were some caveats around quoting (see vshReadlineInit()).
The fix makes sense as is, but I never really understood why
'cmdComplete' even needed readline in the first place.
If you or somebody else wants to attempt dropping it, be my guest. It's
just that currently the code is re-used in both interactive and
non-interactive modes. But maybe there's a way to do all of that
(including quoting) in a way that does not depend on readline.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
Thanks.
Michal
11:18 a.m.
New subject: [PATCH 6/6] vsh: Don't crash when @text is NULL in vshCompleterFilter()
This can happen only for cmdComplete() in interactive mode (which
I'm still not convinced is any useful for users and whether we
should support it). Anyway, running plain 'complete' command with
no additional arguments boils down to @text being NULL in
vshReadlineParse() which handles the case just right but is then
subsequently passed to vshCompleterFilter() which isn't prepared
for this case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index c91d756885..6cc1f60d87 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2820,7 +2820,7 @@ vshCompleterFilter(char ***list,
newList = g_new0(char *, list_len + 1);
for (i = 0; i < list_len; i++) {
- if (!STRPREFIX((*list)[i], text)) {
+ if (text && !STRPREFIX((*list)[i], text)) {
g_clear_pointer(&(*list)[i], g_free);
continue;
}
--
2.44.1
2:57 p.m.
New subject: [PATCH 6/6] vsh: Don't crash when @text is NULL in
vshCompleterFilter()
On Mon, May 27, 2024 at 11:18:54 +0200, Michal Privoznik wrote:
This can happen only for cmdComplete() in interactive mode (which
I'm still not convinced is any useful for users and whether we
should support it).
Definitely not useful for any normal user. Came in handy when testing
stuff.
But this happpens also in non-interactive mode:
$ ./tools/virsh complete
Segmentation fault (core dumped)
Anyway, running plain 'complete' command with
no additional arguments boils down to @text being NULL in
vshReadlineParse() which handles the case just right but is then
subsequently passed to vshCompleterFilter() which isn't prepared
for this case.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/vsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index c91d756885..6cc1f60d87 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2820,7 +2820,7 @@ vshCompleterFilter(char ***list,
newList = g_new0(char *, list_len + 1);
for (i = 0; i < list_len; i++) {
- if (!STRPREFIX((*list)[i], text)) {
+ if (text && !STRPREFIX((*list)[i], text)) {
g_clear_pointer(&(*list)[i], g_free);
continue;
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
115
days inactive
115
days old
14 comments
3 participants
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa