[PATCH 0/6] Random fixes
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
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@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
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@redhat.com> --- docs/kbase/debuglogs.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@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
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@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@redhat.com>
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@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@redhat.com>
Michal
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@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
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@redhat.com> --- tools/bash-completion/vsh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@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
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@redhat.com> --- tools/vsh.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@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
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@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@redhat.com>
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@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@redhat.com>
Thanks. Michal
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@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
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@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@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa