| Message ID | 20161229223815.13705-1-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 29 Dec 2016 20:38:15 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > The "hotplugged" property is user visible, but it was never meant > to be set by the user. There are probably multiple ways to break > or crash device code by overriding the property. One example: > > $ qemu-system-x86_64 -cpu qemu64,hotplugged=true > Segmentation fault (core dumped) > > The DeviceState::hotplugged struct field is set directly by > device_initfn(), there's no need to provide a setter for the > property. this property is meant to be used for individual devices on target side of migration. Doing above is a rather big hammer with behavioral change of migrated instance. So I'd fix crash caused by assumption that hotplugged CPU guarantied to have rtc&fw_cfg available. I'll post a patch with the fix. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/qdev.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 57834423b9..f5989c41cb 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) > return dev->hotplugged; > } > > -static void device_set_hotplugged(Object *obj, bool value, Error **err) > -{ > - DeviceState *dev = DEVICE(obj); > - > - dev->hotplugged = value; > -} > - > static void device_initfn(Object *obj) > { > DeviceState *dev = DEVICE(obj); > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) > object_property_add_bool(obj, "hotpluggable", > device_get_hotpluggable, NULL, NULL); > object_property_add_bool(obj, "hotplugged", > - device_get_hotplugged, device_set_hotplugged, > + device_get_hotplugged, NULL, > &error_abort); > > class = object_get_class(OBJECT(dev));
On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote: > On Thu, 29 Dec 2016 20:38:15 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > The "hotplugged" property is user visible, but it was never meant > > to be set by the user. There are probably multiple ways to break > > or crash device code by overriding the property. One example: > > > > $ qemu-system-x86_64 -cpu qemu64,hotplugged=true > > Segmentation fault (core dumped) > > > > The DeviceState::hotplugged struct field is set directly by > > device_initfn(), there's no need to provide a setter for the > > property. > this property is meant to be used for individual devices on target side > of migration. I didn't know that. Is this documented somewhere? Is it actually used by any existing software? > Doing above is a rather big hammer with behavioral change of migrated > instance. If the property is really supposed to be set directly by users, then I agree. But I would like to understand how exactly it is supposed to work, so we can fix those issues properly. Do you have any existing example where setting "hotplugged=true" on the command-line is necessary and where it really works? > > So I'd fix crash caused by assumption that hotplugged CPU > guarantied to have rtc&fw_cfg available. > I'll post a patch with the fix. Most of the cases where I see DeviceState::hotplugged being used are related to generation of hotplug events[1] or completing steps that are normally done by machine init code[2][3], and I am not sure this should be the case when we're just migrating hotplugged devices. Are all those cases broken, and they should be checking the qdev_hotplug global variable instead? Examples: [1] acpi_cpu_plug_cb(), acpi_pcihp_device_plug_cb(), nvdimm_acpi_plug_cb(); [2] cpu_common_realizefn() calls cpu_synchronize_post_init() and cpu_resume() if dev->hotplug is set. [3] pc_cpu_plug() > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/core/qdev.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 57834423b9..f5989c41cb 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) > > return dev->hotplugged; > > } > > > > -static void device_set_hotplugged(Object *obj, bool value, Error **err) > > -{ > > - DeviceState *dev = DEVICE(obj); > > - > > - dev->hotplugged = value; > > -} > > - > > static void device_initfn(Object *obj) > > { > > DeviceState *dev = DEVICE(obj); > > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) > > object_property_add_bool(obj, "hotpluggable", > > device_get_hotpluggable, NULL, NULL); > > object_property_add_bool(obj, "hotplugged", > > - device_get_hotplugged, device_set_hotplugged, > > + device_get_hotplugged, NULL, > > &error_abort); > > > > class = object_get_class(OBJECT(dev)); >
On Fri, 30 Dec 2016 16:23:08 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote: > > On Thu, 29 Dec 2016 20:38:15 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > The "hotplugged" property is user visible, but it was never meant > > > to be set by the user. There are probably multiple ways to break > > > or crash device code by overriding the property. One example: > > > > > > $ qemu-system-x86_64 -cpu qemu64,hotplugged=true > > > Segmentation fault (core dumped) > > > > > > The DeviceState::hotplugged struct field is set directly by > > > device_initfn(), there's no need to provide a setter for the > > > property. > > this property is meant to be used for individual devices on target side > > of migration. > > I didn't know that. Is this documented somewhere? > Is it actually used by any existing software? not that I know of. But users should be fixed if they are not using it. > > Doing above is a rather big hammer with behavioral change of migrated > > instance. > > If the property is really supposed to be set directly by users, > then I agree. But I would like to understand how exactly it is > supposed to work, so we can fix those issues properly. > > Do you have any existing example where setting "hotplugged=true" > on the command-line is necessary and where it really works? > > > > > So I'd fix crash caused by assumption that hotplugged CPU > > guarantied to have rtc&fw_cfg available. > > I'll post a patch with the fix. > > Most of the cases where I see DeviceState::hotplugged being used > are related to generation of hotplug events[1] or completing > steps that are normally done by machine init code[2][3], and I am > not sure this should be the case when we're just migrating > hotplugged devices. Are all those cases broken, and they should > be checking the qdev_hotplug global variable instead? skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't affect functionality as current ACPI code doesn't depends on it and it's safe to drop hotplug events for already plugged devices as machine init/done code would do the rest of initialization in this case. However if hotplugged property is not set on target then mgmt would loose information that device has been hotplugged when it would query qemu. For example: info memory-devices Memory device [dimm]: "" addr: 0x100000000 slot: 1 node: 0 size: 1073741824 memdev: /objects/mem2 hotplugged: false hotpluggable: true <=== would become false Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm, 'git grep hotplugged' shows that it's used by pci, spapr and other devices and I'm not sure that changing hotplugged from true to false is safe thing to do. > Examples: > > [1] acpi_cpu_plug_cb(), acpi_pcihp_device_plug_cb(), > nvdimm_acpi_plug_cb(); > [2] cpu_common_realizefn() calls cpu_synchronize_post_init() and > cpu_resume() if dev->hotplug is set. > [3] pc_cpu_plug() > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > hw/core/qdev.c | 9 +-------- > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > index 57834423b9..f5989c41cb 100644 > > > --- a/hw/core/qdev.c > > > +++ b/hw/core/qdev.c > > > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) > > > return dev->hotplugged; > > > } > > > > > > -static void device_set_hotplugged(Object *obj, bool value, Error **err) > > > -{ > > > - DeviceState *dev = DEVICE(obj); > > > - > > > - dev->hotplugged = value; > > > -} > > > - > > > static void device_initfn(Object *obj) > > > { > > > DeviceState *dev = DEVICE(obj); > > > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) > > > object_property_add_bool(obj, "hotpluggable", > > > device_get_hotpluggable, NULL, NULL); > > > object_property_add_bool(obj, "hotplugged", > > > - device_get_hotplugged, device_set_hotplugged, > > > + device_get_hotplugged, NULL, > > > &error_abort); > > > > > > class = object_get_class(OBJECT(dev)); > > >
On Tue, Jan 03, 2017 at 02:02:52PM +0100, Igor Mammedov wrote: > On Fri, 30 Dec 2016 16:23:08 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote: > > > On Thu, 29 Dec 2016 20:38:15 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > The "hotplugged" property is user visible, but it was never meant > > > > to be set by the user. There are probably multiple ways to break > > > > or crash device code by overriding the property. One example: > > > > > > > > $ qemu-system-x86_64 -cpu qemu64,hotplugged=true > > > > Segmentation fault (core dumped) > > > > > > > > The DeviceState::hotplugged struct field is set directly by > > > > device_initfn(), there's no need to provide a setter for the > > > > property. > > > this property is meant to be used for individual devices on target side > > > of migration. > > > > I didn't know that. Is this documented somewhere? > > Is it actually used by any existing software? > not that I know of. But users should be fixed if they are not using it. I see. The problem is that the mechanism is undocumented, untested, and seems very likely to trigger bugs in device code. > > > > Doing above is a rather big hammer with behavioral change of migrated > > > instance. > > > > If the property is really supposed to be set directly by users, > > then I agree. But I would like to understand how exactly it is > > supposed to work, so we can fix those issues properly. > > > > Do you have any existing example where setting "hotplugged=true" > > on the command-line is necessary and where it really works? > > > > > > > > So I'd fix crash caused by assumption that hotplugged CPU > > > guarantied to have rtc&fw_cfg available. > > > I'll post a patch with the fix. > > > > Most of the cases where I see DeviceState::hotplugged being used > > are related to generation of hotplug events[1] or completing > > steps that are normally done by machine init code[2][3], and I am > > not sure this should be the case when we're just migrating > > hotplugged devices. Are all those cases broken, and they should > > be checking the qdev_hotplug global variable instead? > skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't > affect functionality as current ACPI code doesn't depends on it > and it's safe to drop hotplug events for already plugged devices > as machine init/done code would do the rest of initialization > in this case. > > However if hotplugged property is not set on target then > mgmt would loose information that device has been hotplugged > when it would query qemu. For example: > info memory-devices > Memory device [dimm]: "" > addr: 0x100000000 > slot: 1 > node: 0 > size: 1073741824 > memdev: /objects/mem2 > hotplugged: false > hotpluggable: true <=== would become false > > > Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm, > 'git grep hotplugged' shows that it's used by > pci, spapr and other devices and I'm not sure that > changing hotplugged from true to false is safe thing to do. Yes. But for the same reason I am not sure that setting hotplugged=on on command-line devices is a safe thing to do. So, we have two problems we want to avoid: A) Not changing hotplugged from true to false on migration; B) Not breaking devices when using "-device ...,hotplugged=true" on the command-line. Problem (A) already exists, as far as I can see, but we never got any bug reports related to it. The fix for (A) you propose is to set "hotplugged=true" on the command-line. This triggered (B) on the CPU code, but your patch fixed it. The fix I proposed for (B) (this patch) prevents your fix for (A) from being implemented. This is a problem. Your patch to fix (B) in the CPU code looks good, but I am worried about other possible instances of (B). So, I propose we do this: 1) Drop this patch, by now; 2) Document properly what management software is supposed to do to fix (A), and test if the proposed solution doesn't break anything (I think we will find other device code to crash or misbehave if using "-device ...,hotplugged=true"); 3) If we don't do (2) in the next QEMU release, I will resubmit this patch to remove the unused/undocumented/untested feature. > > > > Examples: > > > > [1] acpi_cpu_plug_cb(), acpi_pcihp_device_plug_cb(), > > nvdimm_acpi_plug_cb(); > > [2] cpu_common_realizefn() calls cpu_synchronize_post_init() and > > cpu_resume() if dev->hotplug is set. > > [3] pc_cpu_plug() > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > hw/core/qdev.c | 9 +-------- > > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > index 57834423b9..f5989c41cb 100644 > > > > --- a/hw/core/qdev.c > > > > +++ b/hw/core/qdev.c > > > > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) > > > > return dev->hotplugged; > > > > } > > > > > > > > -static void device_set_hotplugged(Object *obj, bool value, Error **err) > > > > -{ > > > > - DeviceState *dev = DEVICE(obj); > > > > - > > > > - dev->hotplugged = value; > > > > -} > > > > - > > > > static void device_initfn(Object *obj) > > > > { > > > > DeviceState *dev = DEVICE(obj); > > > > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) > > > > object_property_add_bool(obj, "hotpluggable", > > > > device_get_hotpluggable, NULL, NULL); > > > > object_property_add_bool(obj, "hotplugged", > > > > - device_get_hotplugged, device_set_hotplugged, > > > > + device_get_hotplugged, NULL, > > > > &error_abort); > > > > > > > > class = object_get_class(OBJECT(dev)); > > > > > >
On Tue, 3 Jan 2017 12:22:23 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jan 03, 2017 at 02:02:52PM +0100, Igor Mammedov wrote: > > On Fri, 30 Dec 2016 16:23:08 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Fri, Dec 30, 2016 at 03:28:34PM +0100, Igor Mammedov wrote: > > > > On Thu, 29 Dec 2016 20:38:15 -0200 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > The "hotplugged" property is user visible, but it was never meant > > > > > to be set by the user. There are probably multiple ways to break > > > > > or crash device code by overriding the property. One example: > > > > > > > > > > $ qemu-system-x86_64 -cpu qemu64,hotplugged=true > > > > > Segmentation fault (core dumped) > > > > > > > > > > The DeviceState::hotplugged struct field is set directly by > > > > > device_initfn(), there's no need to provide a setter for the > > > > > property. > > > > this property is meant to be used for individual devices on target side > > > > of migration. > > > > > > I didn't know that. Is this documented somewhere? > > > Is it actually used by any existing software? > > not that I know of. But users should be fixed if they are not using it. > > I see. The problem is that the mechanism is undocumented, > untested, and seems very likely to trigger bugs in device code. > > > > > > > Doing above is a rather big hammer with behavioral change of migrated > > > > instance. > > > > > > If the property is really supposed to be set directly by users, > > > then I agree. But I would like to understand how exactly it is > > > supposed to work, so we can fix those issues properly. > > > > > > Do you have any existing example where setting "hotplugged=true" > > > on the command-line is necessary and where it really works? > > > > > > > > > > > So I'd fix crash caused by assumption that hotplugged CPU > > > > guarantied to have rtc&fw_cfg available. > > > > I'll post a patch with the fix. > > > > > > Most of the cases where I see DeviceState::hotplugged being used > > > are related to generation of hotplug events[1] or completing > > > steps that are normally done by machine init code[2][3], and I am > > > not sure this should be the case when we're just migrating > > > hotplugged devices. Are all those cases broken, and they should > > > be checking the qdev_hotplug global variable instead? > > skipping hotplugged for x86 CPUs and pc-dimms on target side shouldn't > > affect functionality as current ACPI code doesn't depends on it > > and it's safe to drop hotplug events for already plugged devices > > as machine init/done code would do the rest of initialization > > in this case. > > > > However if hotplugged property is not set on target then > > mgmt would loose information that device has been hotplugged > > when it would query qemu. For example: > > info memory-devices > > Memory device [dimm]: "" > > addr: 0x100000000 > > slot: 1 > > node: 0 > > size: 1073741824 > > memdev: /objects/mem2 > > hotplugged: false > > hotpluggable: true <=== would become false > > > > > > Also beside of simplistic use of hotplugged by x86 cpus/pc-dimm, > > 'git grep hotplugged' shows that it's used by > > pci, spapr and other devices and I'm not sure that > > changing hotplugged from true to false is safe thing to do. > > Yes. But for the same reason I am not sure that setting > hotplugged=on on command-line devices is a safe thing to do. > > So, we have two problems we want to avoid: > A) Not changing hotplugged from true to false on migration; > B) Not breaking devices when using > "-device ...,hotplugged=true" on the command-line. > > Problem (A) already exists, as far as I can see, but we never got > any bug reports related to it. > > The fix for (A) you propose is to set "hotplugged=true" on the > command-line. This triggered (B) on the CPU code, but your patch > fixed it. > > The fix I proposed for (B) (this patch) prevents your fix for (A) > from being implemented. This is a problem. > > Your patch to fix (B) in the CPU code looks good, but I am > worried about other possible instances of (B). > > So, I propose we do this: > > 1) Drop this patch, by now; agreed > 2) Document properly what management software is supposed > to do to fix (A), and test if the proposed solution doesn't Do you have in mind where to document it? I'll add to loop libvirt folks. > break anything (I think we will find other device code to > crash or misbehave if using "-device ...,hotplugged=true"); As for tests, I'm not sure I'll be able to cover all usecases, but I can take care of x86 cpus and pc/nv-dimms. -device path is unlikely to trigger bugs as it's executed rather late when machine init code is completed and basically tested by real device hotplug path. Your example of x86 cpu crash is unexpected usage of -global foo.hotplugged=on which is applied early in machine init time to all devices of given type which looks wrong to me. I'd propose to prevent using -global for essentially per-instance property. > 3) If we don't do (2) in the next QEMU release, I will resubmit > this patch to remove the unused/undocumented/untested feature. Applying this patch looks wrong to me regardless of (2) for already mentioned reasons. We should at least document property usage and ask libvirt to use it if it's not using it yet. > > > > > > > > Examples: > > > > > > [1] acpi_cpu_plug_cb(), acpi_pcihp_device_plug_cb(), > > > nvdimm_acpi_plug_cb(); > > > [2] cpu_common_realizefn() calls cpu_synchronize_post_init() and > > > cpu_resume() if dev->hotplug is set. > > > [3] pc_cpu_plug() > > > > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > --- > > > > > hw/core/qdev.c | 9 +-------- > > > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > index 57834423b9..f5989c41cb 100644 > > > > > --- a/hw/core/qdev.c > > > > > +++ b/hw/core/qdev.c > > > > > @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) > > > > > return dev->hotplugged; > > > > > } > > > > > > > > > > -static void device_set_hotplugged(Object *obj, bool value, Error **err) > > > > > -{ > > > > > - DeviceState *dev = DEVICE(obj); > > > > > - > > > > > - dev->hotplugged = value; > > > > > -} > > > > > - > > > > > static void device_initfn(Object *obj) > > > > > { > > > > > DeviceState *dev = DEVICE(obj); > > > > > @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) > > > > > object_property_add_bool(obj, "hotpluggable", > > > > > device_get_hotpluggable, NULL, NULL); > > > > > object_property_add_bool(obj, "hotplugged", > > > > > - device_get_hotplugged, device_set_hotplugged, > > > > > + device_get_hotplugged, NULL, > > > > > &error_abort); > > > > > > > > > > class = object_get_class(OBJECT(dev)); > > > > > > > > > >
On 03/01/2017 15:22, Eduardo Habkost wrote: >> I didn't know that. Is this documented somewhere? >> Is it actually used by any existing software? > not that I know of. But users should be fixed if they are not using it. > > I see. The problem is that the mechanism is undocumented, > untested, and seems very likely to trigger bugs in device code. I agree. Why can't hotplugged be migrated? Paolo
On Tue, 3 Jan 2017 17:10:15 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 03/01/2017 15:22, Eduardo Habkost wrote: > >> I didn't know that. Is this documented somewhere? > >> Is it actually used by any existing software? > > not that I know of. But users should be fixed if they are not using it. > > > > I see. The problem is that the mechanism is undocumented, > > untested, and seems very likely to trigger bugs in device code. > > I agree. Why can't hotplugged be migrated? It's probably not migrated because of it's not runtime/guest modified state so we don't have to migrate it as it's know in advance. For now it should set manually on CLI (-device) with the rest of hotplugged device properties. > Paolo
(CCing libvir-list and Laine) On Tue, Jan 03, 2017 at 05:53:27PM +0100, Igor Mammedov wrote: > On Tue, 3 Jan 2017 17:10:15 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > On 03/01/2017 15:22, Eduardo Habkost wrote: > > >> I didn't know that. Is this documented somewhere? > > >> Is it actually used by any existing software? > > > not that I know of. But users should be fixed if they are not using it. > > > > > > I see. The problem is that the mechanism is undocumented, > > > untested, and seems very likely to trigger bugs in device code. > > > > I agree. Why can't hotplugged be migrated? > It's probably not migrated because of it's not runtime/guest modified > state so we don't have to migrate it as it's know in advance. > > For now it should set manually on CLI (-device) with the rest of > hotplugged device properties. As this recommendation has the potential to trigger hidden bugs (and known to trigger a bug in QEMU <= 2.8), I would like it to be properly documented, and the documentation/recommendations reviewed following the usual patch review process. While we don't do that, setting "hotplugged=true" on the command-line is an unused, undocumented, untested (and unsupported?) feature.
On Tue, Jan 03, 2017 at 03:06:02PM -0200, Eduardo Habkost wrote: > (CCing libvir-list and Laine) > > On Tue, Jan 03, 2017 at 05:53:27PM +0100, Igor Mammedov wrote: > > On Tue, 3 Jan 2017 17:10:15 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > > On 03/01/2017 15:22, Eduardo Habkost wrote: > > > >> I didn't know that. Is this documented somewhere? > > > >> Is it actually used by any existing software? > > > > not that I know of. But users should be fixed if they are not using it. > > > > > > > > I see. The problem is that the mechanism is undocumented, > > > > untested, and seems very likely to trigger bugs in device code. > > > > > > I agree. Why can't hotplugged be migrated? > > It's probably not migrated because of it's not runtime/guest modified > > state so we don't have to migrate it as it's know in advance. > > > > For now it should set manually on CLI (-device) with the rest of > > hotplugged device properties. > > As this recommendation has the potential to trigger hidden bugs > (and known to trigger a bug in QEMU <= 2.8), I would like it to > be properly documented, and the documentation/recommendations > reviewed following the usual patch review process. > > While we don't do that, setting "hotplugged=true" on the > command-line is an unused, undocumented, untested (and > unsupported?) feature. As the ability to set hotplugged=true is still unused and undocumented, I will resubmit this patch to disable the feature in QEMU 2.9.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 57834423b9..f5989c41cb 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -1013,13 +1013,6 @@ static bool device_get_hotplugged(Object *obj, Error **err) return dev->hotplugged; } -static void device_set_hotplugged(Object *obj, bool value, Error **err) -{ - DeviceState *dev = DEVICE(obj); - - dev->hotplugged = value; -} - static void device_initfn(Object *obj) { DeviceState *dev = DEVICE(obj); @@ -1039,7 +1032,7 @@ static void device_initfn(Object *obj) object_property_add_bool(obj, "hotpluggable", device_get_hotpluggable, NULL, NULL); object_property_add_bool(obj, "hotplugged", - device_get_hotplugged, device_set_hotplugged, + device_get_hotplugged, NULL, &error_abort); class = object_get_class(OBJECT(dev));
The "hotplugged" property is user visible, but it was never meant to be set by the user. There are probably multiple ways to break or crash device code by overriding the property. One example: $ qemu-system-x86_64 -cpu qemu64,hotplugged=true Segmentation fault (core dumped) The DeviceState::hotplugged struct field is set directly by device_initfn(), there's no need to provide a setter for the property. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/core/qdev.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)