Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
Thomas Huth 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 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 :)
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
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 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
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
On Tue, Mar 21, 2023 at 09:47:57PM +0100, Paolo Bonzini wrote: > Il lun 20 mar 2023, 16:42 Thomas Huth 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 -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
Il lun 20 mar 2023, 16:42 Thomas Huth 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 > >
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
On Mon, 20 Mar 2023 17:05:07 +0100
Markus Armbruster wrote:
> Thomas Huth writes:
>
> > On 20/03/2023 16.31, Markus Armbruster wrote:
> >> Claudio Imbrenda 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
> >>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on
> >>> Linux")
> >>> Signed-off-by: Claudio Imbrenda
> >>> ---
> >>> 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
> >>> #include
> >>> #include
> >>> @@ -144,3 +147,17 @@ void init_async_teardown(void)
> >>> clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
> >>> sigprocmask(SIG_SETMASK, _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(_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)
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
Thomas Huth writes:
> On 20/03/2023 16.31, Markus Armbruster wrote:
>> Claudio Imbrenda 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
>>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
>>> Signed-off-by: Claudio Imbrenda
>>> ---
>>> 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
>>> #include
>>> #include
>>> @@ -144,3 +147,17 @@ void init_async_teardown(void)
>>> clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
>>> sigprocmask(SIG_SETMASK, _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(_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?
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
On 20/03/2023 16.31, Markus Armbruster wrote:
Claudio Imbrenda 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
Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
Signed-off-by: Claudio Imbrenda
---
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
#include
#include
@@ -144,3 +147,17 @@ void init_async_teardown(void)
clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
sigprocmask(SIG_SETMASK, _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(_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
Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
Claudio Imbrenda 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
> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
> Signed-off-by: Claudio Imbrenda
> ---
> 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
> #include
> #include
> @@ -144,3 +147,17 @@ void init_async_teardown(void)
> clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
> sigprocmask(SIG_SETMASK, _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(_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.
[PATCH v2 1/1] util/async-teardown: wire up query-command-line-options
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
Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
Signed-off-by: Claudio Imbrenda
---
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
#include
#include
@@ -144,3 +147,17 @@ void init_async_teardown(void)
clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
sigprocmask(SIG_SETMASK, _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(_async_teardown_opts);
+}
+opts_init(register_async_teardown);
--
2.39.2
