| Message ID | 1360096768-8863-6-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <lersek@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. > 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 > >
On Thu, 7 Feb 2013 15:12:22 -0200 Eduardo Habkost <ehabkost@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 <lersek@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 > > > > >
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Thu, 7 Feb 2013 15:12:22 -0200 > Eduardo Habkost <ehabkost@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 <lersek@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. I prefer plain "if (err)". >> > 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. Yes, it does the right thing. Weaker preconditions are good :) >> Maybe we should extend the error_propagate() documentation to mention it >> is OK to call error_propagate() even if error is unset? Makes sense. [...]
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)) { 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); } 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)) { qerror_report_err(err); error_free(err); exit(1);
Problems are now reported via Error only. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-properties.h | 4 ++-- hw/qdev-monitor.c | 3 ++- hw/qdev-properties.c | 14 ++++++-------- 3 files changed, 10 insertions(+), 11 deletions(-)