| Message ID | 20230320131648.61728-2-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | util/async-teardown: wire up query-command-line-options | expand |
Claudio Imbrenda <imbrenda@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 <fiuczy@linux.ibm.com> > Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux") > Signed-off-by: Claudio Imbrenda <imbrenda@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.
On 20/03/2023 16.31, Markus Armbruster wrote: > Claudio Imbrenda <imbrenda@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 <fiuczy@linux.ibm.com> >> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux") >> Signed-off-by: Claudio Imbrenda <imbrenda@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" ? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 20/03/2023 16.31, Markus Armbruster wrote: >> Claudio Imbrenda <imbrenda@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 <fiuczy@linux.ibm.com> >>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux") >>> Signed-off-by: Claudio Imbrenda <imbrenda@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?
On Mon, 20 Mar 2023 17:05:07 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Thomas Huth <thuth@redhat.com> writes: > > > On 20/03/2023 16.31, Markus Armbruster wrote: > >> Claudio Imbrenda <imbrenda@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 <fiuczy@linux.ibm.com> > >>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux") > >>> Signed-off-by: Claudio Imbrenda <imbrenda@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)
Il lun 20 mar 2023, 16:42 Thomas Huth <thuth@redhat.com> ha scritto: > Would it make sense to add it e.g. to "-action" instead, i.e. something > like > "-action teardown=async" ? > -action is just a wrapper for the action-set QMP command. I don't think it fits very well; its arguments are only guest actions while asynchronous tear down happens for example when you issue a quit command on the monitor. Paolo Thomas > >
On Tue, Mar 21, 2023 at 09:47:57PM +0100, Paolo Bonzini wrote: > Il lun 20 mar 2023, 16:42 Thomas Huth <thuth@redhat.com> ha scritto: > > > Would it make sense to add it e.g. to "-action" instead, i.e. something > > like > > "-action teardown=async" ? > > > > -action is just a wrapper for the action-set QMP command. I don't think it > fits very well; its arguments are only guest actions while asynchronous > tear down happens for example when you issue a quit command on the monitor. Right, we discussed -action when this feature was first proposed and that was the reason it was discounted. With regards, Daniel
On 22/03/2023 10.28, Daniel P. Berrangé wrote: > On Tue, Mar 21, 2023 at 09:47:57PM +0100, Paolo Bonzini wrote: >> Il lun 20 mar 2023, 16:42 Thomas Huth <thuth@redhat.com> ha scritto: >> >>> Would it make sense to add it e.g. to "-action" instead, i.e. something >>> like >>> "-action teardown=async" ? >>> >> >> -action is just a wrapper for the action-set QMP command. I don't think it >> fits very well; its arguments are only guest actions while asynchronous >> tear down happens for example when you issue a quit command on the monitor. > > Right, we discussed -action when this feature was first proposed and > that was the reason it was discounted. I guess that was this thread here : https://mail.gnu.org/archive/html/qemu-devel/2022-08/msg04479.html ? Anyway, how to continue now here? If I've got that right, we currently need an option that takes a parameter if we want to make it visible via QAPI, right? So maybe remove the previous option (since it cannot be used by upper layer like libvirt anyway yet), and introduce a new one like "-teardown async|sync" ? Or rework the current one into "-async-teardown on|off" (similar to "-sandbox on")? Any preferences? Or do we want something even more generic instead, e.g.: -run-with teardown=async -run-with daemonized=on -run-with chroot=/path/to/chroot/dir -run-with userid=UID ... so we could get rid of -deamonize, -chroot and -runas and other similar options one day? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 22/03/2023 10.28, Daniel P. Berrangé wrote: >> On Tue, Mar 21, 2023 at 09:47:57PM +0100, Paolo Bonzini wrote: >>> Il lun 20 mar 2023, 16:42 Thomas Huth <thuth@redhat.com> ha scritto: >>> >>>> Would it make sense to add it e.g. to "-action" instead, i.e. something >>>> like >>>> "-action teardown=async" ? >>>> >>> >>> -action is just a wrapper for the action-set QMP command. I don't think it >>> fits very well; its arguments are only guest actions while asynchronous >>> tear down happens for example when you issue a quit command on the monitor. >> Right, we discussed -action when this feature was first proposed and >> that was the reason it was discounted. > > I guess that was this thread here : > > https://mail.gnu.org/archive/html/qemu-devel/2022-08/msg04479.html > > ? > > Anyway, how to continue now here? If I've got that right, we currently need an option that takes a parameter if we want to make it visible via QAPI, right? Visible in query-command-line-options, to be precise. To make it visible in query-qmp-schema, you need to make it a QMP command in addition to a CLI option. Would that be possible? > So maybe remove the previous option (since it cannot be used by upper layer like libvirt anyway yet), and introduce a new one like "-teardown async|sync" ? Or rework the current one into "-async-teardown on|off" (similar to "-sandbox on")? Any preferences? > > Or do we want something even more generic instead, e.g.: > > -run-with teardown=async > -run-with daemonized=on > -run-with chroot=/path/to/chroot/dir > -run-with userid=UID > > ... so we could get rid of -deamonize, -chroot and -runas and other similar options one day? If we expect more teardown-relation configuration knobs, then something like -teardown async=[on|off] would let us add more with ease. -run-with is a bit of a grab bag. We've done worse :)
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);
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. 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 <fiuczy@linux.ibm.com> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- util/async-teardown.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)