On Thu, 7 Feb 2013 15:12:22 -0200
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Tue, Feb 05, 2013 at 09:39:18PM +0100, Laszlo Ersek wrote:
> > Problems are now reported via Error only.
> >
> > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > ---
> > hw/qdev-properties.h | 4 ++--
> > hw/qdev-monitor.c | 3 ++-
> > hw/qdev-properties.c | 14 ++++++--------
> > 3 files changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> > index 0fe4030..43fd11a 100644
> > --- a/hw/qdev-properties.h
> > +++ b/hw/qdev-properties.h
> > @@ -100,8 +100,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
> >
> > /* Set properties between creation and init. */
> > void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
> > -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> > - Error **errp);
> > +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> > + Error **errp);
> > void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
> > void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t
> > value);
> > void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t
> > value);
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index 5eb1c8c..cf96046 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -110,7 +110,8 @@ static int set_property(const char *name, const char
> > *value, void *opaque)
> > if (strcmp(name, "bus") == 0)
> > return 0;
> >
> > - if (qdev_prop_parse(dev, name, value, &err) == -1) {
> > + qdev_prop_parse(dev, name, value, &err);
> > + if (error_is_set(&err)) {
>
> You can use "if (err)" instead. I believe it is acceptable, as there's
> lots of (recently introduced) QEMU code using this style.
Yes, people tend to use if (err) in new code. For me it honestly doesn't
matter much, although I wonder if we should have a more strict rule for this.
>
>
> > qerror_report_err(err);
> > error_free(err);
> > return -1;
> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > index 8e3d014..d94909e 100644
> > --- a/hw/qdev-properties.c
> > +++ b/hw/qdev-properties.c
> > @@ -835,8 +835,8 @@ void error_set_from_qdev_prop_error(Error **errp, int
> > ret, DeviceState *dev,
> > }
> > }
> >
> > -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> > - Error **errp)
> > +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> > + Error **errp)
> > {
> > char *legacy_name;
> > Error *err = NULL;
> > @@ -849,11 +849,7 @@ int qdev_prop_parse(DeviceState *dev, const char
> > *name, const char *value,
> > }
> > g_free(legacy_name);
> >
> > - if (err) {
> > - error_propagate(errp, err);
> > - return -1;
> > - }
> > - return 0;
> > + error_propagate(errp, err);
>
> I didn't expect it to be valid to call error_propagate() if error is
> _not_ set. All cases of error_propagate() usage I have seen before first
> checked if error was set. But by looking at the implementation, it seems
> to be OK.
>
> Maybe we should extend the error_propagate() documentation to mention it
> is OK to call error_propagate() even if error is unset?
>
>
> > }
> >
> > void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
> > @@ -967,7 +963,9 @@ void qdev_prop_set_globals(DeviceState *dev)
> > if (strcmp(object_class_get_name(class), prop->driver) != 0) {
> > continue;
> > }
> > - if (qdev_prop_parse(dev, prop->property, prop->value, &err) !=
> > 0) {
> > +
> > + qdev_prop_parse(dev, prop->property, prop->value, &err);
> > + if (error_is_set(&err)) {
>
> You can use "if (err)" here, too.
>
> > qerror_report_err(err);
> > error_free(err);
> > exit(1);
> > --
> > 1.7.1
> >
> >
>
