| Message ID | 20170222192647.19690-1-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/22/2017 01:26 PM, Eduardo Habkost 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. For example, we > recently fixed a crash in rtc_set_memory() related to the > property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d). > > There has been some discussion about making management software > use "hotplugged=on" on migration, to indicate devices that were > hotplugged in the migration source. There were other suggestions > to address this, like including the "hotplugged" field in the > migration stream instead of requiring it to be set propertly. s/propertly/properly/ > > Whatever solution we choose in the future, this patch disables > setting "hotplugged" explicitly in the command-line by now, > because the ability to set the property is unused, untested, and > undocumented. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/qdev.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 06ba02e2a3..800c9ca23f 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -1016,13 +1016,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); > @@ -1042,7 +1035,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 Wed, Feb 22, 2017 at 04:43:37PM -0600, Eric Blake wrote: > On 02/22/2017 01:26 PM, Eduardo Habkost 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. For example, we > > recently fixed a crash in rtc_set_memory() related to the > > property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d). > > > > There has been some discussion about making management software > > use "hotplugged=on" on migration, to indicate devices that were > > hotplugged in the migration source. There were other suggestions > > to address this, like including the "hotplugged" field in the > > migration stream instead of requiring it to be set propertly. > > s/propertly/properly/ Actually, I think I meant "requiring it to be set explicitly". I will fix this in my branch. Thanks! > > > > > Whatever solution we choose in the future, this patch disables > > setting "hotplugged" explicitly in the command-line by now, > > because the ability to set the property is unused, untested, and > > undocumented. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/core/qdev.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks! > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 06ba02e2a3..800c9ca23f 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -1016,13 +1016,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); > > @@ -1042,7 +1035,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)); > > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Eduardo Habkost <ehabkost@redhat.com> writes: > 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. For example, we > recently fixed a crash in rtc_set_memory() related to the > property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d). > > There has been some discussion about making management software > use "hotplugged=on" on migration, to indicate devices that were > hotplugged in the migration source. There were other suggestions > to address this, like including the "hotplugged" field in the > migration stream instead of requiring it to be set propertly. > > Whatever solution we choose in the future, this patch disables > setting "hotplugged" explicitly in the command-line by now, > because the ability to set the property is unused, untested, and > undocumented. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> I figure this is the safe thing to do for 2.9. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Mon, Feb 27, 2017 at 07:05:24PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > 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. For example, we > > recently fixed a crash in rtc_set_memory() related to the > > property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d). > > > > There has been some discussion about making management software > > use "hotplugged=on" on migration, to indicate devices that were > > hotplugged in the migration source. There were other suggestions > > to address this, like including the "hotplugged" field in the > > migration stream instead of requiring it to be set propertly. > > > > Whatever solution we choose in the future, this patch disables > > setting "hotplugged" explicitly in the command-line by now, > > because the ability to set the property is unused, untested, and > > undocumented. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > I figure this is the safe thing to do for 2.9. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks. I have merged it to my "machine" branch, and I plan to include it in a pull request next week.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 06ba02e2a3..800c9ca23f 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -1016,13 +1016,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); @@ -1042,7 +1035,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. For example, we recently fixed a crash in rtc_set_memory() related to the property (commit 26ef65beab852caf2b1ef4976e3473f2d525164d). There has been some discussion about making management software use "hotplugged=on" on migration, to indicate devices that were hotplugged in the migration source. There were other suggestions to address this, like including the "hotplugged" field in the migration stream instead of requiring it to be set propertly. Whatever solution we choose in the future, this patch disables setting "hotplugged" explicitly in the command-line by now, because the ability to set the property is unused, untested, and undocumented. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/core/qdev.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)