* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 12:38 ` [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax Daniel P. Berrangé
@ 2022-01-05 12:49 ` Thomas Huth
2022-01-05 14:37 ` Ján Tomko
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2022-01-05 12:49 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Peter Krempa, Markus Armbruster,
Eduardo Habkost, Paolo Bonzini, Eric Blake
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 12:38 ` [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax Daniel P. Berrangé
2022-01-05 12:49 ` Thomas Huth
@ 2022-01-05 14:37 ` Ján Tomko
2022-01-05 14:49 ` Laurent Vivier
2022-01-14 12:22 ` Markus Armbruster
3 siblings, 0 replies; 12+ messages in thread
From: Ján Tomko @ 2022-01-05 14:37 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, Peter Krempa,
qemu-devel, Markus Armbruster, Eduardo Habkost, Paolo Bonzini,
Eric Blake
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 12:38 ` [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax Daniel P. Berrangé
2022-01-05 12:49 ` Thomas Huth
2022-01-05 14:37 ` Ján Tomko
@ 2022-01-05 14:49 ` Laurent Vivier
2022-01-05 14:55 ` Daniel P. Berrangé
2022-01-14 12:22 ` Markus Armbruster
3 siblings, 1 reply; 12+ messages in thread
From: Laurent Vivier @ 2022-01-05 14:49 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, Thomas Huth, Peter Krempa,
Markus Armbruster, Paolo Bonzini, Eric Blake
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 14:49 ` Laurent Vivier
@ 2022-01-05 14:55 ` Daniel P. Berrangé
2022-01-05 15:00 ` Laurent Vivier
0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2022-01-05 14:55 UTC (permalink / raw)
To: Laurent Vivier
Cc: Kevin Wolf, Eduardo Habkost, Thomas Huth, Peter Krempa,
qemu-devel, Markus Armbruster, Paolo Bonzini, Eric Blake
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 14:55 ` Daniel P. Berrangé
@ 2022-01-05 15:00 ` Laurent Vivier
2022-01-05 15:18 ` Daniel P. Berrangé
0 siblings, 1 reply; 12+ messages in thread
From: Laurent Vivier @ 2022-01-05 15:00 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Eduardo Habkost, Thomas Huth, Peter Krempa,
qemu-devel, Markus Armbruster, Paolo Bonzini, Eric Blake
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 15:00 ` Laurent Vivier
@ 2022-01-05 15:18 ` Daniel P. Berrangé
2022-01-05 15:19 ` Thomas Huth
2022-01-05 15:31 ` Laurent Vivier
0 siblings, 2 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2022-01-05 15:18 UTC (permalink / raw)
To: Laurent Vivier
Cc: Kevin Wolf, Eduardo Habkost, Thomas Huth, Peter Krempa,
qemu-devel, Markus Armbruster, Paolo Bonzini, Eric Blake
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 15:18 ` Daniel P. Berrangé
@ 2022-01-05 15:19 ` Thomas Huth
2022-01-05 15:31 ` Laurent Vivier
1 sibling, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2022-01-05 15:19 UTC (permalink / raw)
To: Daniel P. Berrangé, Laurent Vivier
Cc: Kevin Wolf, Eduardo Habkost, Peter Krempa, qemu-devel,
Markus Armbruster, Paolo Bonzini, Eric Blake
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 15:18 ` Daniel P. Berrangé
2022-01-05 15:19 ` Thomas Huth
@ 2022-01-05 15:31 ` Laurent Vivier
1 sibling, 0 replies; 12+ messages in thread
From: Laurent Vivier @ 2022-01-05 15:31 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Eduardo Habkost, Thomas Huth, Peter Krempa,
qemu-devel, Markus Armbruster, Paolo Bonzini, Eric Blake
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax
2022-01-05 12:38 ` [PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax Daniel P. Berrangé
` (2 preceding siblings ...)
2022-01-05 14:49 ` Laurent Vivier
@ 2022-01-14 12:22 ` Markus Armbruster
3 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2022-01-14 12:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, Peter Krempa,
qemu-devel, Eduardo Habkost, Paolo Bonzini, Eric Blake
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>
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread