| Message ID | 1364218844-7509-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | b000dfbd42e8aac9af66ae0ed787ef1021bea29f |
| Headers | show |
On Mar 25, 2013 5:40 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > > Now we have error_setg() we can improve the error message emitted if > you attempt to set a property of a device after the device is realized > (the previous message was "permission denied" which was not very > informative). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Another attempt at updating this patch (v3 October, v2 July). > Changes v3->v4: > * rebased; some functions have moved file, so the 'set the error' > function can't be file-local. Renamed it to something more suitable > for a public function and added a doc comment. > * added set_taddr to the set of errors changed (and also checked we > now have the complete set) > > Not changed: Igor suggested using "after it was initialized" rather > than "after it was realized"; I think the original text is better, > because (a) we're moving towards having all devices implement realize > rather than init and (b) it's mostly an error message for QEMU developers > rather than end-users, so the technical terminology is not inappropriate. > > hw/qdev-addr.c | 2 +- > hw/qdev-properties-system.c | 4 ++-- > hw/qdev-properties.c | 40 +++++++++++++++++++++++++++------------- > hw/qdev-properties.h | 12 ++++++++++++ > hw/qdev.c | 2 +- > 5 files changed, 43 insertions(+), 17 deletions(-) > Reviewed-by: Igor Mitsyanko <i.mitsyanko@gmail.com> > diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c > index 2398b4a..80a38bb 100644 > --- a/hw/qdev-addr.c > +++ b/hw/qdev-addr.c > @@ -42,7 +42,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque, > int64_t value; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c > index 8795144..28813d3 100644 > --- a/hw/qdev-properties-system.c > +++ b/hw/qdev-properties-system.c > @@ -43,7 +43,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > int ret; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -287,7 +287,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque, > NetClientState *hubport; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 247ca6c..168c466 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -7,6 +7,20 @@ > #include "qapi/visitor.h" > #include "char/char.h" > > +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, > + Error **errp) > +{ > + if (dev->id) { > + error_setg(errp, "Attempt to set property '%s' on device '%s' " > + "(type '%s') after it was realized", name, dev->id, > + object_get_typename(OBJECT(dev))); > + } else { > + error_setg(errp, "Attempt to set property '%s' on anonymous device " > + "(type '%s') after it was realized", name, > + object_get_typename(OBJECT(dev))); > + } > +} > + > void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) > { > void *ptr = dev; > @@ -33,7 +47,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque, > int *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -86,7 +100,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque, > bool value; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -126,7 +140,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque, > uint8_t *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -193,7 +207,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque, > uint16_t *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -226,7 +240,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque, > uint32_t *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -251,7 +265,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque, > int32_t *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -324,7 +338,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque, > uint64_t *ptr = qdev_get_prop_ptr(dev, prop); > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -414,7 +428,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque, > char *str; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -478,7 +492,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque, > char *str, *p; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -570,7 +584,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque, > char *str; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -641,7 +655,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, > const int64_t max = 32768; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -709,7 +723,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque, > unsigned int slot = 0, func = 0; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > @@ -824,7 +838,7 @@ static void set_prop_arraylen(Object *obj, Visitor *v, void *opaque, > int i; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > if (*alenptr) { > diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h > index c9bb246..a379339 100644 > --- a/hw/qdev-properties.h > +++ b/hw/qdev-properties.h > @@ -167,4 +167,16 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev, > */ > void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp); > > +/** > + * @qdev_prop_set_after_realize: > + * @dev: device > + * @name: name of property > + * @errp: indirect pointer to Error to be set > + * Set the Error object to report that an attempt was made to set a property > + * on a device after it has already been realized. This is a utility function > + * which allows property-setter functions to easily report the error in > + * a friendly format identifying both the device and the property. > + */ > +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, > + Error **errp); > #endif > diff --git a/hw/qdev.c b/hw/qdev.c > index 909c405..6b1947e 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -562,7 +562,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque, > int ret; > > if (dev->realized) { > - error_set(errp, QERR_PERMISSION_DENIED); > + qdev_prop_set_after_realize(dev, name, errp); > return; > } > > -- > 1.7.9.5 >
Applied. Thanks. Regards, Anthony Liguori
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c index 2398b4a..80a38bb 100644 --- a/hw/qdev-addr.c +++ b/hw/qdev-addr.c @@ -42,7 +42,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque, int64_t value; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c index 8795144..28813d3 100644 --- a/hw/qdev-properties-system.c +++ b/hw/qdev-properties-system.c @@ -43,7 +43,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, int ret; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -287,7 +287,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque, NetClientState *hubport; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 247ca6c..168c466 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -7,6 +7,20 @@ #include "qapi/visitor.h" #include "char/char.h" +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, + Error **errp) +{ + if (dev->id) { + error_setg(errp, "Attempt to set property '%s' on device '%s' " + "(type '%s') after it was realized", name, dev->id, + object_get_typename(OBJECT(dev))); + } else { + error_setg(errp, "Attempt to set property '%s' on anonymous device " + "(type '%s') after it was realized", name, + object_get_typename(OBJECT(dev))); + } +} + void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) { void *ptr = dev; @@ -33,7 +47,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque, int *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -86,7 +100,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque, bool value; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -126,7 +140,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque, uint8_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -193,7 +207,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque, uint16_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -226,7 +240,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque, uint32_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -251,7 +265,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque, int32_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -324,7 +338,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque, uint64_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -414,7 +428,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque, char *str; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -478,7 +492,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque, char *str, *p; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -570,7 +584,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque, char *str; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -641,7 +655,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, const int64_t max = 32768; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -709,7 +723,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque, unsigned int slot = 0, func = 0; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -824,7 +838,7 @@ static void set_prop_arraylen(Object *obj, Visitor *v, void *opaque, int i; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } if (*alenptr) { diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h index c9bb246..a379339 100644 --- a/hw/qdev-properties.h +++ b/hw/qdev-properties.h @@ -167,4 +167,16 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev, */ void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp); +/** + * @qdev_prop_set_after_realize: + * @dev: device + * @name: name of property + * @errp: indirect pointer to Error to be set + * Set the Error object to report that an attempt was made to set a property + * on a device after it has already been realized. This is a utility function + * which allows property-setter functions to easily report the error in + * a friendly format identifying both the device and the property. + */ +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, + Error **errp); #endif diff --git a/hw/qdev.c b/hw/qdev.c index 909c405..6b1947e 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -562,7 +562,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque, int ret; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; }
Now we have error_setg() we can improve the error message emitted if you attempt to set a property of a device after the device is realized (the previous message was "permission denied" which was not very informative). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Another attempt at updating this patch (v3 October, v2 July). Changes v3->v4: * rebased; some functions have moved file, so the 'set the error' function can't be file-local. Renamed it to something more suitable for a public function and added a doc comment. * added set_taddr to the set of errors changed (and also checked we now have the complete set) Not changed: Igor suggested using "after it was initialized" rather than "after it was realized"; I think the original text is better, because (a) we're moving towards having all devices implement realize rather than init and (b) it's mostly an error message for QEMU developers rather than end-users, so the technical terminology is not inappropriate. hw/qdev-addr.c | 2 +- hw/qdev-properties-system.c | 4 ++-- hw/qdev-properties.c | 40 +++++++++++++++++++++++++++------------- hw/qdev-properties.h | 12 ++++++++++++ hw/qdev.c | 2 +- 5 files changed, 43 insertions(+), 17 deletions(-)