On Mon, 20 Mar 2023 17:05:07 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Thomas Huth <th...@redhat.com> writes: > > > On 20/03/2023 16.31, Markus Armbruster wrote: > >> Claudio Imbrenda <imbre...@linux.ibm.com> writes: > >> > >>> The recently introduced -async-teardown commandline option was not > >>> wired up properly and did not show up in the output of the QMP command > >>> query-command-line-options. This means that libvirt will have no way to > >>> discover whether the feature is supported. > >> > >> There was nothing improper in its wiring. The issue is that > >> query-command-line-options is junk. See my recent post > >> > >> Subject: query-command-line-options (was: [PATCH 1/7] qemu: > >> capabilities: Introduce QEMU_CAPS_MACHINE_ACPI) > >> Date: Tue, 07 Mar 2023 10:40:23 +0100 > >> Message-ID: <87jzzsc320.fsf...@pond.sub.org> > >> > >>> This patch fixes the issue by correctly wiring up the commandline > >>> option so that it appears in the output of query-command-line-options. > >>> > >>> Reported-by: Boris Fiuczynski <fiu...@linux.ibm.com> > >>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on > >>> Linux") > >>> Signed-off-by: Claudio Imbrenda <imbre...@linux.ibm.com> > >>> --- > >>> util/async-teardown.c | 17 +++++++++++++++++ > >>> 1 file changed, 17 insertions(+) > >>> > >>> diff --git a/util/async-teardown.c b/util/async-teardown.c > >>> index 62cdeb0f20..c9b9a3cdb2 100644 > >>> --- a/util/async-teardown.c > >>> +++ b/util/async-teardown.c > >>> @@ -12,6 +12,9 @@ > >>> */ > >>> > >>> #include "qemu/osdep.h" > >>> +#include "qemu/config-file.h" > >>> +#include "qemu/option.h" > >>> +#include "qemu/module.h" > >>> #include <dirent.h> > >>> #include <sys/prctl.h> > >>> #include <sched.h> > >>> @@ -144,3 +147,17 @@ void init_async_teardown(void) > >>> clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL); > >>> sigprocmask(SIG_SETMASK, &old_signals, NULL); > >>> } > >>> + > >>> +static QemuOptsList qemu_async_teardown_opts = { > >>> + .name = "async-teardown", > >>> + .head = QTAILQ_HEAD_INITIALIZER(qemu_async_teardown_opts.head), > >>> + .desc = { > >>> + { /* end of list */ } > >>> + }, > >>> +}; > >>> + > >>> +static void register_async_teardown(void) > >>> +{ > >>> + qemu_add_opts(&qemu_async_teardown_opts); > >>> +} > >>> +opts_init(register_async_teardown); > >> > >> Now it *is* improperly wired up :) > >> > >> You're defining new QemuOpts config group "async-teardown" with > >> arbitrary option parameters, but don't actually use it for parsing or > >> recording the option. I figure because you can't: there is no option > >> argument to parse and record, which is what QemuOpts is designed to do. > >> > >> If you need the feature to be visible in query-command-line-options, you > >> should make it an option parameter (a KEY, not a GROUP), preferably of > >> an existing group / option. > > > > Would it make sense to add it e.g. to "-action" instead, i.e. something > > like > > "-action teardown=async" ? > > I believe the new parameter "teardown" would be visible in > query-command-line-options. > > How well does it fit -action? I guess it can be shoehorned in. generally action is about stuff that happens in/to the guest, while in this case it's about how qemu will perform the teardown of its address space once it terminates. the important parts are: this is an OS-specific option (Linux), and it needs to be parsed and enabled before sandboxing (otherwise clone(2) might not work)