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