[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 05/15] qdev_prop_parse(): change return type
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 05/15] qdev_prop_parse(): change return type to void |
Date: |
Wed, 20 Mar 2013 14:58:11 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Luiz Capitulino <address@hidden> writes:
> On Thu, 7 Feb 2013 15:12:22 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>
>> > ---
>> > 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.
[...]
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 05/15] qdev_prop_parse(): change return type to void,
Markus Armbruster <=