[PATCH v2 0/1] util/async-teardown: wire up query-command-line-options

[PATCH v2 0/1] util/async-teardown: wire up query-command-line-options

[Claudio Imbrenda]

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.

v1->v2
* remove the unneeded .implied_opt_name initializer [Thomas]

Claudio Imbrenda (1):
  util/async-teardown: wire up query-command-line-options

 util/async-teardown.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

-- 
2.39.2

[PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Claudio Imbrenda]

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(+)

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);
-- 
2.39.2

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Markus Armbruster]

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.

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Thomas Huth]

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

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Markus Armbruster]

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?

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Claudio Imbrenda]

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)

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 1126 bytes --]

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Daniel P. Berrangé]

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
-- 
|: 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

[Thomas Huth]

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

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Markus Armbruster]

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 :)