Message ID | 1403788203-9364-1-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > When a device is unparented (i.e. made completely hidden from management) > we want to send a DEVICE_DELETED event only if the device actually was > realized. This avoids raising DEVICE_DELETED events when device_add > fails. > > However, this does not work right for recursively-deleted > devices: the whole tree is _first_ unrealized, _then_ unparented. > Then device_unparent sees realized==false and fails to trigger > the event. The solution is simply to move have_realized into > the DeviceState struct. If device_add fails, we never set the > new field to true and DEVICE_DELETED is not sent. > > Fixes qemu-iotests testcase 067. Suggest to add "Broken in commit 5942a19" here, to make it clear that it's a recent regression. > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/core/qdev.c | 5 +++-- > include/hw/qdev-core.h | 1 + > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index d1eba3c..c520415 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -848,6 +848,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (value && !dev->realized) { [...] > if (dev->hotplugged && local_err == NULL) { > device_reset(dev); > } > + dev->pending_deleted_event = false; Unset on completion of unrealized -> realized transition. > } else if (!value && dev->realized) { > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > object_property_set_bool(OBJECT(bus), false, "realized", > @@ -862,6 +863,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > if (dc->unrealize && local_err == NULL) { > dc->unrealize(dev, &local_err); > } > + dev->pending_deleted_event = true; Set on completion of realized -> unrealized transition. > } > > if (local_err != NULL) { > @@ -972,7 +974,6 @@ static void device_unparent(Object *obj) > { > DeviceState *dev = DEVICE(obj); > BusState *bus; > - bool have_realized = dev->realized; > > if (dev->realized) { > object_property_set_bool(obj, false, "realized", NULL); > @@ -988,7 +989,7 @@ static void device_unparent(Object *obj) > } > > /* Only send event if the device had been completely realized */ > - if (have_realized) { > + if (dev->pending_deleted_event) { > gchar *path = object_get_canonical_path(OBJECT(dev)); > > qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort); Let's see whether I understand how this works. Please correct misunderstandings. device_unparent() runs right before device deletion, and only then. First thing it does is setting property "realized" to false. Does nothing if the device has never been completely realized. dev->pending_deleted_event remains in its initial state false. DEVICE_DELETED not sent. Good. Else, the device was completely realized at some time. If it is currently realized, we get a transition to unrealized right now, setting dev->pending_deleted_event. Else, the last transition must have been realized -> unrealized, setting dev->pending_deleted_event. Since it gets unset only on unrealized -> realized, it's still set. Therefore, dev->pending_deleted_event is set if and only if the device has been completely realized. > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 9221cfc..0799ff2 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -156,6 +156,7 @@ struct DeviceState { > > const char *id; > bool realized; > + bool pending_deleted_event; > QemuOpts *opts; > int hotplugged; > BusState *parent_bus; Reviewed-by: Markus Armbruster <armbru@redhat.com> (Tested, too, but my r-by subsumes that here)
Am 27.06.2014 09:16, schrieb Markus Armbruster: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> When a device is unparented (i.e. made completely hidden from management) >> we want to send a DEVICE_DELETED event only if the device actually was >> realized. This avoids raising DEVICE_DELETED events when device_add >> fails. >> >> However, this does not work right for recursively-deleted >> devices: the whole tree is _first_ unrealized, _then_ unparented. >> Then device_unparent sees realized==false and fails to trigger >> the event. The solution is simply to move have_realized into >> the DeviceState struct. If device_add fails, we never set the >> new field to true and DEVICE_DELETED is not sent. >> >> Fixes qemu-iotests testcase 067. > > Suggest to add "Broken in commit 5942a19" here, to make it clear that > it's a recent regression. I vaguely recall that something like this was in Bandan's RFC (that I assume the above commit forward-ported, the subject would be handy to mention too), but once again without any explanation why, so I saw no need to apply that during hardfreeze. Andreas >> Reported-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
Andreas Färber <afaerber@suse.de> writes: > Am 27.06.2014 09:16, schrieb Markus Armbruster: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> When a device is unparented (i.e. made completely hidden from management) >>> we want to send a DEVICE_DELETED event only if the device actually was >>> realized. This avoids raising DEVICE_DELETED events when device_add >>> fails. >>> >>> However, this does not work right for recursively-deleted >>> devices: the whole tree is _first_ unrealized, _then_ unparented. >>> Then device_unparent sees realized==false and fails to trigger >>> the event. The solution is simply to move have_realized into >>> the DeviceState struct. If device_add fails, we never set the >>> new field to true and DEVICE_DELETED is not sent. >>> >>> Fixes qemu-iotests testcase 067. >> >> Suggest to add "Broken in commit 5942a19" here, to make it clear that >> it's a recent regression. > > I vaguely recall that something like this was in Bandan's RFC (that I > assume the above commit forward-ported, the subject would be handy to > mention too), but once again without any explanation why, so I saw no > need to apply that during hardfreeze. Thanks for pointing it out, yes, I believe that my RFC had this case covered. (For historical accuracy) Can we please include a link to the RFC in the commit message ? Thanks, Bandan > Andreas > >>> Reported-by: Markus Armbruster <armbru@redhat.com> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index d1eba3c..c520415 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -848,6 +848,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (dev->hotplugged && local_err == NULL) { device_reset(dev); } + dev->pending_deleted_event = false; } else if (!value && dev->realized) { QLIST_FOREACH(bus, &dev->child_bus, sibling) { object_property_set_bool(OBJECT(bus), false, "realized", @@ -862,6 +863,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (dc->unrealize && local_err == NULL) { dc->unrealize(dev, &local_err); } + dev->pending_deleted_event = true; } if (local_err != NULL) { @@ -972,7 +974,6 @@ static void device_unparent(Object *obj) { DeviceState *dev = DEVICE(obj); BusState *bus; - bool have_realized = dev->realized; if (dev->realized) { object_property_set_bool(obj, false, "realized", NULL); @@ -988,7 +989,7 @@ static void device_unparent(Object *obj) } /* Only send event if the device had been completely realized */ - if (have_realized) { + if (dev->pending_deleted_event) { gchar *path = object_get_canonical_path(OBJECT(dev)); qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 9221cfc..0799ff2 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -156,6 +156,7 @@ struct DeviceState { const char *id; bool realized; + bool pending_deleted_event; QemuOpts *opts; int hotplugged; BusState *parent_bus;
When a device is unparented (i.e. made completely hidden from management) we want to send a DEVICE_DELETED event only if the device actually was realized. This avoids raising DEVICE_DELETED events when device_add fails. However, this does not work right for recursively-deleted devices: the whole tree is _first_ unrealized, _then_ unparented. Then device_unparent sees realized==false and fails to trigger the event. The solution is simply to move have_realized into the DeviceState struct. If device_add fails, we never set the new field to true and DEVICE_DELETED is not sent. Fixes qemu-iotests testcase 067. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/qdev.c | 5 +++-- include/hw/qdev-core.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-)