Patchwork [v2,05/15] qdev_prop_parse(): change return type to void

login
register
mail settings
Submitter Laszlo Ersek
Date Feb. 5, 2013, 8:39 p.m.
Message ID <1360096768-8863-6-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/218349/
State New
Headers show

Comments

Laszlo Ersek - Feb. 5, 2013, 8:39 p.m.
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(-)
Eduardo Habkost - Feb. 7, 2013, 5:12 p.m.
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
> 
>
Luiz Capitulino - Feb. 7, 2013, 5:58 p.m.
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
> > 
> > 
>
Markus Armbruster - March 20, 2013, 1:58 p.m.
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.

[...]

Patch

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);