[v2,03/15] qdev_prop_parse(): extend signature with Error

Submitted by Laszlo Ersek on Feb. 5, 2013, 8:39 p.m.

Details

Message ID 1360096768-8863-4-git-send-email-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Feb. 5, 2013, 8:39 p.m.
Error handling is not changed yet.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-properties.h |    4 +++-
 hw/qdev-monitor.c    |    2 +-
 hw/qdev-properties.c |    5 +++--
 3 files changed, 7 insertions(+), 4 deletions(-)

Comments

Eduardo Habkost Feb. 7, 2013, 5:04 p.m.
On Tue, Feb 05, 2013 at 09:39:16PM +0100, Laszlo Ersek wrote:
> Error handling is not changed yet.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

The extra parameter seems useless and even misleading (because Error
info won't be set even if the function returns -1) without patch 04/15.
What about squashing both patches together?


> ---
>  hw/qdev-properties.h |    4 +++-
>  hw/qdev-monitor.c    |    2 +-
>  hw/qdev-properties.c |    5 +++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> index 20c67f3..0fe4030 100644
> --- a/hw/qdev-properties.h
> +++ b/hw/qdev-properties.h
> @@ -2,6 +2,7 @@
>  #define QEMU_QDEV_PROPERTIES_H
>  
>  #include "qdev-core.h"
> +#include "qapi/error.h"
>  
>  /*** qdev-properties.c ***/
>  
> @@ -99,7 +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);
> +int 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 bbdc90f..02297e1 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -109,7 +109,7 @@ 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) == -1) {
> +    if (qdev_prop_parse(dev, name, value, NULL) == -1) {
>          return -1;
>      }
>      return 0;
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index a8a31f5..e2dbbbe 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -835,7 +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)
> +int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> +                    Error **errp)
>  {
>      char *legacy_name;
>      Error *err = NULL;
> @@ -965,7 +966,7 @@ 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) != 0) {
> +            if (qdev_prop_parse(dev, prop->property, prop->value, NULL) != 0) {
>                  exit(1);
>              }
>          }
> -- 
> 1.7.1
> 
>
Luiz Capitulino Feb. 7, 2013, 5:52 p.m.
On Thu, 7 Feb 2013 15:04:54 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Feb 05, 2013 at 09:39:16PM +0100, Laszlo Ersek wrote:
> > Error handling is not changed yet.
> > 
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> The extra parameter seems useless and even misleading (because Error
> info won't be set even if the function returns -1) without patch 04/15.
> What about squashing both patches together?

I suggested him to do it this way as a general rule because it makes
it easier to review when the function getting the Error ** argument is
called from several other functions. In that case, you'll usually add
different error handling to callers and having everything in one big
patch makes it just impossible to review.

This case is simple, so it doesn't matter much. Although I do think
it's easier to review this way.

Patch hide | download patch | download mbox

diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
index 20c67f3..0fe4030 100644
--- a/hw/qdev-properties.h
+++ b/hw/qdev-properties.h
@@ -2,6 +2,7 @@ 
 #define QEMU_QDEV_PROPERTIES_H
 
 #include "qdev-core.h"
+#include "qapi/error.h"
 
 /*** qdev-properties.c ***/
 
@@ -99,7 +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);
+int 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 bbdc90f..02297e1 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -109,7 +109,7 @@  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) == -1) {
+    if (qdev_prop_parse(dev, name, value, NULL) == -1) {
         return -1;
     }
     return 0;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index a8a31f5..e2dbbbe 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -835,7 +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)
+int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
+                    Error **errp)
 {
     char *legacy_name;
     Error *err = NULL;
@@ -965,7 +966,7 @@  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) != 0) {
+            if (qdev_prop_parse(dev, prop->property, prop->value, NULL) != 0) {
                 exit(1);
             }
         }