[PATCH 0/1] Fix -device JSON support wrt hotplug

[PATCH 0/1] Fix -device JSON support wrt hotplug

[Daniel P. Berrangé]

Libvirt switched to using -device JSON support, but we discovered in
testing that it is broken for hotplug, never sending DEVICE_DELETED
events. This is caused by a subtle refcount leak.

Daniel P. Berrangé (1):
  softmmu: fix device deletion events with -device JSON syntax

 qapi/qdev.json                 |  5 ++++-
 softmmu/vl.c                   |  4 +++-
 tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.33.1

[PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Daniel P. Berrangé]

The -device JSON syntax impl leaks a reference on the created
DeviceState instance. As a result when you hot-unplug the
device, the device_finalize method won't be called and thus
it will fail to emit the required DEVICE_DELETED event.

A 'json-cli' feature was previously added against the
'device_add' QMP command QAPI schema to indicated to mgmt
apps that -device supported JSON syntax. Given the hotplug
bug that feature flag is no unusable for its purpose, so
we add a new 'json-cli-hotplug' feature to indicate the
-device supports JSON without breaking hotplug.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/qdev.json                 |  5 ++++-
 softmmu/vl.c                   |  4 +++-
 tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 69656b14df..26cd10106b 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -44,6 +44,9 @@
 # @json-cli: If present, the "-device" command line option supports JSON
 #            syntax with a structure identical to the arguments of this
 #            command.
+# @json-cli-hotplug: If present, the "-device" command line option supports JSON
+#                    syntax without the reference counting leak that broke
+#                    hot-unplug
 #
 # Notes:
 #
@@ -74,7 +77,7 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli'] }
+  'features': ['json-cli', 'json-cli-hotplug'] }
 
 ##
 # @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index d9e4c619d3..b1fc7da104 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
+        DeviceState *dev;
         loc_push_restore(&opt->loc);
         /*
          * TODO Eventually we should call qmp_device_add() here to make sure it
@@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
          * from the start, so call qdev_device_add_from_qdict() directly for
          * now.
          */
-        qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
+        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
+        object_unref(OBJECT(dev));
         loc_pop(&opt->loc);
     }
     rom_reset_order_override();
diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index 559d47727a..ad79bd4c14 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
     qtest_quit(qtest);
 }
 
+static void test_pci_unplug_json_request(void)
+{
+    QTestState *qtest = qtest_initf(
+        "-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
+
+    /*
+     * Request device removal. As the guest is not running, the request won't
+     * be processed. However during system reset, the removal will be
+     * handled, removing the device.
+     */
+    device_del(qtest, "dev0");
+    system_reset(qtest);
+    wait_device_deleted_event(qtest, "dev0");
+
+    qtest_quit(qtest);
+}
+
 static void test_ccw_unplug(void)
 {
     QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0");
@@ -145,6 +162,8 @@ int main(int argc, char **argv)
      */
     qtest_add_func("/device-plug/pci-unplug-request",
                    test_pci_unplug_request);
+    qtest_add_func("/device-plug/pci-unplug-json-request",
+                   test_pci_unplug_json_request);
 
     if (!strcmp(arch, "s390x")) {
         qtest_add_func("/device-plug/ccw-unplug",
-- 
2.33.1

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Thomas Huth]

On 05/01/2022 13.38, Daniel P. Berrangé wrote:
> The -device JSON syntax impl leaks a reference on the created
> DeviceState instance. As a result when you hot-unplug the
> device, the device_finalize method won't be called and thus
> it will fail to emit the required DEVICE_DELETED event.
> 
> A 'json-cli' feature was previously added against the
> 'device_add' QMP command QAPI schema to indicated to mgmt
> apps that -device supported JSON syntax. Given the hotplug
> bug that feature flag is no unusable for its purpose, so
> we add a new 'json-cli-hotplug' feature to indicate the
> -device supports JSON without breaking hotplug.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802

We're mostly using "Fixes:" to refer to previous commit IDs, and "Resolves:" 
for referring to bugs in the gitlab issue tracker, so in case you respin, 
I'd suggest to replace it (but both keywords should work to close issues, so 
it's just a cosmetical thing).

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   qapi/qdev.json                 |  5 ++++-
>   softmmu/vl.c                   |  4 +++-
>   tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>   3 files changed, 26 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Ján Tomko]

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

On a Wednesday in 2022, Daniel P. Berrangé wrote:
>The -device JSON syntax impl leaks a reference on the created
>DeviceState instance. As a result when you hot-unplug the
>device, the device_finalize method won't be called and thus
>it will fail to emit the required DEVICE_DELETED event.
>
>A 'json-cli' feature was previously added against the
>'device_add' QMP command QAPI schema to indicated to mgmt
>apps that -device supported JSON syntax. Given the hotplug
>bug that feature flag is no unusable for its purpose, so
>we add a new 'json-cli-hotplug' feature to indicate the
>-device supports JSON without breaking hotplug.
>
>Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> qapi/qdev.json                 |  5 ++++-
> softmmu/vl.c                   |  4 +++-
> tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
>

Tested-by: Ján Tomko <jtomko@redhat.com>

Jano

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Laurent Vivier]

On 05/01/2022 13:38, Daniel P. Berrangé wrote:
> The -device JSON syntax impl leaks a reference on the created
> DeviceState instance. As a result when you hot-unplug the
> device, the device_finalize method won't be called and thus
> it will fail to emit the required DEVICE_DELETED event.
> 
> A 'json-cli' feature was previously added against the
> 'device_add' QMP command QAPI schema to indicated to mgmt
> apps that -device supported JSON syntax. Given the hotplug
> bug that feature flag is no unusable for its purpose, so

Not sure to understand: do you mean "now unusable"?

> we add a new 'json-cli-hotplug' feature to indicate the
> -device supports JSON without breaking hotplug.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   qapi/qdev.json                 |  5 ++++-
>   softmmu/vl.c                   |  4 +++-
>   tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>   3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 69656b14df..26cd10106b 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -44,6 +44,9 @@
>   # @json-cli: If present, the "-device" command line option supports JSON
>   #            syntax with a structure identical to the arguments of this
>   #            command.
> +# @json-cli-hotplug: If present, the "-device" command line option supports JSON
> +#                    syntax without the reference counting leak that broke
> +#                    hot-unplug
>   #
>   # Notes:
>   #
> @@ -74,7 +77,7 @@
>   { 'command': 'device_add',
>     'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
>     'gen': false, # so we can get the additional arguments
> -  'features': ['json-cli'] }
> +  'features': ['json-cli', 'json-cli-hotplug'] }
>   
>   ##
>   # @device_del:
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index d9e4c619d3..b1fc7da104 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
>       qemu_opts_foreach(qemu_find_opts("device"),
>                         device_init_func, NULL, &error_fatal);
>       QTAILQ_FOREACH(opt, &device_opts, next) {
> +        DeviceState *dev;
>           loc_push_restore(&opt->loc);
>           /*
>            * TODO Eventually we should call qmp_device_add() here to make sure it
> @@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
>            * from the start, so call qdev_device_add_from_qdict() directly for
>            * now.
>            */
> -        qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> +        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> +        object_unref(OBJECT(dev));
>           loc_pop(&opt->loc);
>       }
>       rom_reset_order_override();
> diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> index 559d47727a..ad79bd4c14 100644
> --- a/tests/qtest/device-plug-test.c
> +++ b/tests/qtest/device-plug-test.c
> @@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
>       qtest_quit(qtest);
>   }
>   
> +static void test_pci_unplug_json_request(void)
> +{
> +    QTestState *qtest = qtest_initf(
> +        "-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
> +
> +    /*
> +     * Request device removal. As the guest is not running, the request won't
> +     * be processed. However during system reset, the removal will be
> +     * handled, removing the device.
> +     */
> +    device_del(qtest, "dev0");
> +    system_reset(qtest);

You can use qpci_unplug_acpi_device_test() to process the request... but I see this is 
done like that too in test_pci_unplug_request()

> +    wait_device_deleted_event(qtest, "dev0");
> +
> +    qtest_quit(qtest);
> +}
> +
>   static void test_ccw_unplug(void)
>   {
>       QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0");
> @@ -145,6 +162,8 @@ int main(int argc, char **argv)
>        */
>       qtest_add_func("/device-plug/pci-unplug-request",
>                      test_pci_unplug_request);
> +    qtest_add_func("/device-plug/pci-unplug-json-request",
> +                   test_pci_unplug_json_request);
>   
>       if (!strcmp(arch, "s390x")) {
>           qtest_add_func("/device-plug/ccw-unplug",

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Daniel P. Berrangé]

On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
> On 05/01/2022 13:38, Daniel P. Berrangé wrote:
> > The -device JSON syntax impl leaks a reference on the created
> > DeviceState instance. As a result when you hot-unplug the
> > device, the device_finalize method won't be called and thus
> > it will fail to emit the required DEVICE_DELETED event.
> > 
> > A 'json-cli' feature was previously added against the
> > 'device_add' QMP command QAPI schema to indicated to mgmt
> > apps that -device supported JSON syntax. Given the hotplug
> > bug that feature flag is no unusable for its purpose, so
> 
> Not sure to understand: do you mean "now unusable"?

An application wants to known whether QEMU can support JSON
syntax with -device. If they look for the 'json-cli' feature
as a witness, they'll end up using JSON with QEMU 6.2 which
is giving them broken hotplug. This is unusable for any
non-trivial use cases. So we need a new witness to indicate
whether JSON is viable with -device, that only the newly
fixed QEMU will report.

> 
> > we add a new 'json-cli-hotplug' feature to indicate the
> > -device supports JSON without breaking hotplug.
> > 
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   qapi/qdev.json                 |  5 ++++-
> >   softmmu/vl.c                   |  4 +++-
> >   tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
> >   3 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > index 69656b14df..26cd10106b 100644
> > --- a/qapi/qdev.json
> > +++ b/qapi/qdev.json
> > @@ -44,6 +44,9 @@
> >   # @json-cli: If present, the "-device" command line option supports JSON
> >   #            syntax with a structure identical to the arguments of this
> >   #            command.
> > +# @json-cli-hotplug: If present, the "-device" command line option supports JSON
> > +#                    syntax without the reference counting leak that broke
> > +#                    hot-unplug
> >   #
> >   # Notes:
> >   #
> > @@ -74,7 +77,7 @@
> >   { 'command': 'device_add',
> >     'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
> >     'gen': false, # so we can get the additional arguments
> > -  'features': ['json-cli'] }
> > +  'features': ['json-cli', 'json-cli-hotplug'] }
> >   ##
> >   # @device_del:
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index d9e4c619d3..b1fc7da104 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
> >       qemu_opts_foreach(qemu_find_opts("device"),
> >                         device_init_func, NULL, &error_fatal);
> >       QTAILQ_FOREACH(opt, &device_opts, next) {
> > +        DeviceState *dev;
> >           loc_push_restore(&opt->loc);
> >           /*
> >            * TODO Eventually we should call qmp_device_add() here to make sure it
> > @@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
> >            * from the start, so call qdev_device_add_from_qdict() directly for
> >            * now.
> >            */
> > -        qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> > +        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> > +        object_unref(OBJECT(dev));
> >           loc_pop(&opt->loc);
> >       }
> >       rom_reset_order_override();
> > diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> > index 559d47727a..ad79bd4c14 100644
> > --- a/tests/qtest/device-plug-test.c
> > +++ b/tests/qtest/device-plug-test.c
> > @@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
> >       qtest_quit(qtest);
> >   }
> > +static void test_pci_unplug_json_request(void)
> > +{
> > +    QTestState *qtest = qtest_initf(
> > +        "-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
> > +
> > +    /*
> > +     * Request device removal. As the guest is not running, the request won't
> > +     * be processed. However during system reset, the removal will be
> > +     * handled, removing the device.
> > +     */
> > +    device_del(qtest, "dev0");
> > +    system_reset(qtest);
> 
> You can use qpci_unplug_acpi_device_test() to process the request... but I
> see this is done like that too in test_pci_unplug_request()
> 
> > +    wait_device_deleted_event(qtest, "dev0");
> > +
> > +    qtest_quit(qtest);
> > +}
> > +
> >   static void test_ccw_unplug(void)
> >   {
> >       QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0");
> > @@ -145,6 +162,8 @@ int main(int argc, char **argv)
> >        */
> >       qtest_add_func("/device-plug/pci-unplug-request",
> >                      test_pci_unplug_request);
> > +    qtest_add_func("/device-plug/pci-unplug-json-request",
> > +                   test_pci_unplug_json_request);
> >       if (!strcmp(arch, "s390x")) {
> >           qtest_add_func("/device-plug/ccw-unplug",
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 

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 1/1] softmmu: fix device deletion events with -device JSON syntax

[Laurent Vivier]

On 05/01/2022 15:55, Daniel P. Berrangé wrote:
> On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
>> On 05/01/2022 13:38, Daniel P. Berrangé wrote:
>>> The -device JSON syntax impl leaks a reference on the created
>>> DeviceState instance. As a result when you hot-unplug the
>>> device, the device_finalize method won't be called and thus
>>> it will fail to emit the required DEVICE_DELETED event.
>>>
>>> A 'json-cli' feature was previously added against the
>>> 'device_add' QMP command QAPI schema to indicated to mgmt
>>> apps that -device supported JSON syntax. Given the hotplug
>>> bug that feature flag is no unusable for its purpose, so
>>
>> Not sure to understand: do you mean "now unusable"?
> 
> An application wants to known whether QEMU can support JSON
> syntax with -device. If they look for the 'json-cli' feature
> as a witness, they'll end up using JSON with QEMU 6.2 which
> is giving them broken hotplug. This is unusable for any
> non-trivial use cases. So we need a new witness to indicate
> whether JSON is viable with -device, that only the newly
> fixed QEMU will report.

I understand that, my problem was with your sentence:

"Given the hotplug bug that feature flag is no unusable for its purpose"

Thanks,
Laurent

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Daniel P. Berrangé]

On Wed, Jan 05, 2022 at 04:00:54PM +0100, Laurent Vivier wrote:
> On 05/01/2022 15:55, Daniel P. Berrangé wrote:
> > On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
> > > On 05/01/2022 13:38, Daniel P. Berrangé wrote:
> > > > The -device JSON syntax impl leaks a reference on the created
> > > > DeviceState instance. As a result when you hot-unplug the
> > > > device, the device_finalize method won't be called and thus
> > > > it will fail to emit the required DEVICE_DELETED event.
> > > > 
> > > > A 'json-cli' feature was previously added against the
> > > > 'device_add' QMP command QAPI schema to indicated to mgmt
> > > > apps that -device supported JSON syntax. Given the hotplug
> > > > bug that feature flag is no unusable for its purpose, so
> > > 
> > > Not sure to understand: do you mean "now unusable"?
> > 
> > An application wants to known whether QEMU can support JSON
> > syntax with -device. If they look for the 'json-cli' feature
> > as a witness, they'll end up using JSON with QEMU 6.2 which
> > is giving them broken hotplug. This is unusable for any
> > non-trivial use cases. So we need a new witness to indicate
> > whether JSON is viable with -device, that only the newly
> > fixed QEMU will report.
> 
> I understand that, my problem was with your sentence:
> 
> "Given the hotplug bug that feature flag is no unusable for its purpose"

What's the problem with that ? It is reasonabled to say a -device impl
which is broken for hotplug is not usable for non-toy use cases.

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 1/1] softmmu: fix device deletion events with -device JSON syntax

[Thomas Huth]

On 05/01/2022 16.18, Daniel P. Berrangé wrote:
> On Wed, Jan 05, 2022 at 04:00:54PM +0100, Laurent Vivier wrote:
>> On 05/01/2022 15:55, Daniel P. Berrangé wrote:
>>> On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
>>>> On 05/01/2022 13:38, Daniel P. Berrangé wrote:
>>>>> The -device JSON syntax impl leaks a reference on the created
>>>>> DeviceState instance. As a result when you hot-unplug the
>>>>> device, the device_finalize method won't be called and thus
>>>>> it will fail to emit the required DEVICE_DELETED event.
>>>>>
>>>>> A 'json-cli' feature was previously added against the
>>>>> 'device_add' QMP command QAPI schema to indicated to mgmt
>>>>> apps that -device supported JSON syntax. Given the hotplug
>>>>> bug that feature flag is no unusable for its purpose, so
>>>>
>>>> Not sure to understand: do you mean "now unusable"?
>>>
>>> An application wants to known whether QEMU can support JSON
>>> syntax with -device. If they look for the 'json-cli' feature
>>> as a witness, they'll end up using JSON with QEMU 6.2 which
>>> is giving them broken hotplug. This is unusable for any
>>> non-trivial use cases. So we need a new witness to indicate
>>> whether JSON is viable with -device, that only the newly
>>> fixed QEMU will report.
>>
>> I understand that, my problem was with your sentence:
>>
>> "Given the hotplug bug that feature flag is no unusable for its purpose"
> 
> What's the problem with that ? It is reasonabled to say a -device impl
> which is broken for hotplug is not usable for non-toy use cases.

So it should be "not usable" instead of "no usable" in the commit description.

  Thomas

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Laurent Vivier]

On 05/01/2022 16:18, Daniel P. Berrangé wrote:
> On Wed, Jan 05, 2022 at 04:00:54PM +0100, Laurent Vivier wrote:
>> On 05/01/2022 15:55, Daniel P. Berrangé wrote:
>>> On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
>>>> On 05/01/2022 13:38, Daniel P. Berrangé wrote:
>>>>> The -device JSON syntax impl leaks a reference on the created
>>>>> DeviceState instance. As a result when you hot-unplug the
>>>>> device, the device_finalize method won't be called and thus
>>>>> it will fail to emit the required DEVICE_DELETED event.
>>>>>
>>>>> A 'json-cli' feature was previously added against the
>>>>> 'device_add' QMP command QAPI schema to indicated to mgmt
>>>>> apps that -device supported JSON syntax. Given the hotplug
>>>>> bug that feature flag is no unusable for its purpose, so
>>>>
>>>> Not sure to understand: do you mean "now unusable"?
>>>
>>> An application wants to known whether QEMU can support JSON
>>> syntax with -device. If they look for the 'json-cli' feature
>>> as a witness, they'll end up using JSON with QEMU 6.2 which
>>> is giving them broken hotplug. This is unusable for any
>>> non-trivial use cases. So we need a new witness to indicate
>>> whether JSON is viable with -device, that only the newly
>>> fixed QEMU will report.
>>
>> I understand that, my problem was with your sentence:
>>
>> "Given the hotplug bug that feature flag is no unusable for its purpose"
> 
> What's the problem with that ? It is reasonabled to say a -device impl
> which is broken for hotplug is not usable for non-toy use cases.

The problem for me is the double negation: "no" and "unusable"

Thanks,
Laurent

Re: [PATCH 0/1] Fix -device JSON support wrt hotplug

[Kevin Wolf]

Am 05.01.2022 um 13:38 hat Daniel P. Berrangé geschrieben:
> Libvirt switched to using -device JSON support, but we discovered in
> testing that it is broken for hotplug, never sending DEVICE_DELETED
> events. This is caused by a subtle refcount leak.

Oops, so I fell again in the trap of this interface returning a strong
reference... Maybe it's just me, but I find it highly unintuitive.

Thanks, applied to the block branch.

Kevin

Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The -device JSON syntax impl leaks a reference on the created
> DeviceState instance. As a result when you hot-unplug the
> device, the device_finalize method won't be called and thus
> it will fail to emit the required DEVICE_DELETED event.
>
> A 'json-cli' feature was previously added against the
> 'device_add' QMP command QAPI schema to indicated to mgmt
> apps that -device supported JSON syntax. Given the hotplug
> bug that feature flag is no unusable for its purpose, so

As Laurent and Thomas pointed out, this should be "is not usable" or "is
unusable".

> we add a new 'json-cli-hotplug' feature to indicate the
> -device supports JSON without breaking hotplug.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/qdev.json                 |  5 ++++-
>  softmmu/vl.c                   |  4 +++-
>  tests/qtest/device-plug-test.c | 19 +++++++++++++++++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 69656b14df..26cd10106b 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -44,6 +44,9 @@
>  # @json-cli: If present, the "-device" command line option supports JSON
>  #            syntax with a structure identical to the arguments of this
>  #            command.
> +# @json-cli-hotplug: If present, the "-device" command line option supports JSON
> +#                    syntax without the reference counting leak that broke
> +#                    hot-unplug

For local consistency, please end the sentence with a period and wrap
lines like so:

   # @json-cli-hotplug: If present, the "-device" command line option supports
   #                    JSON syntax without the reference counting leak that
   #                    broke hot-unplug.

>  #
>  # Notes:
>  #
> @@ -74,7 +77,7 @@
>  { 'command': 'device_add',
>    'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
>    'gen': false, # so we can get the additional arguments
> -  'features': ['json-cli'] }
> +  'features': ['json-cli', 'json-cli-hotplug'] }
>  
>  ##
>  # @device_del:

Kevin, I hope you can apply these touch-ups in your tree.  Then, QAPI
schema
Acked-by: Markus Armbruster <armbru@redhat.com>

[...]