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

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

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

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.