Re: [Qemu-devel] [PATCH v2 7/7] qdev-monitor: Propagate errors through qdev_device_add()

[Markus Armbruster]

Andreas Färber <address@hidden> writes:

> Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
>> Also polish an error message while I'm touching the line anyway,
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>  include/monitor/qdev.h |  2 +-
>>  qdev-monitor.c         | 36 +++++++++++++++---------------------
>>  vl.c                   |  7 +++++--
>>  3 files changed, 21 insertions(+), 24 deletions(-)
>> 
>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>> index 7190752..2ce8578 100644
>> --- a/include/monitor/qdev.h
>> +++ b/include/monitor/qdev.h
>> @@ -11,6 +11,6 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict);
>>  void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
>>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>  int qdev_device_help(QemuOpts *opts);
>> -DeviceState *qdev_device_add(QemuOpts *opts);
>> +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
>>  
>>  #endif
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index e7e9a50..686c9df 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -516,7 +516,7 @@ static BusState *qbus_find(const char *path, Error 
>> **errp)
>>      return bus;
>>  }
>>  
>> -DeviceState *qdev_device_add(QemuOpts *opts)
>> +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>  {
>>      DeviceClass *dc;
>>      const char *driver, *path, *id;
>> @@ -526,44 +526,38 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>  
>>      driver = qemu_opt_get(opts, "driver");
>>      if (!driver) {
>> -        qerror_report(QERR_MISSING_PARAMETER, "driver");
>> +        error_set(errp, QERR_MISSING_PARAMETER, "driver");
>>          return NULL;
>>      }
>>  
>>      /* find driver */
>> -    dc = qdev_get_device_class(&driver, &err);
>> -    if (err) {
>> -        qerror_report_err(err);
>> -        error_free(err);
>> +    dc = qdev_get_device_class(&driver, errp);
>> +    if (!dc) {
>
> Here ...
>
>>          return NULL;
>>      }
>>  
>>      /* find bus */
>>      path = qemu_opt_get(opts, "bus");
>>      if (path != NULL) {
>> -        bus = qbus_find(path, &err);
>> +        bus = qbus_find(path, errp);
>>          if (!bus) {
>> -            qerror_report_err(err);
>> -            error_free(err);
>
> ... and here you've elegantly eliminated assumptions about return value
> and errp relations. Before, we were accessing err without checking that
> it is non-NULL - error_free() shouldn't care but qerror_report_err()
> might and just out of design principle.
>
>>              return NULL;
>>          }
>>          if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
>> -            qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> -                          "Device '%s' can't go on a %s bus",
>> -                          driver, object_get_typename(OBJECT(bus)));
>> +            error_setg(errp, "Device '%s' can't go on %s bus",
>> +                       driver, object_get_typename(OBJECT(bus)));
>>              return NULL;
>>          }
>>      } else if (dc->bus_type != NULL) {
>>          bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
>>          if (!bus || qbus_is_full(bus)) {
>> -            qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> -                          "No '%s' bus found for device '%s'",
>> -                          dc->bus_type, driver);
>> +            error_setg(errp, "No '%s' bus found for device '%s'",
>> +                       dc->bus_type, driver);
>>              return NULL;
>>          }
>>      }
>>      if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>> -        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>> +        error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>>          return NULL;
>>      }
>>  
>> @@ -592,7 +586,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>  
>>      /* set properties */
>>      if (qemu_opt_foreach(opts, set_property, dev, &err)) {
>> -        qerror_report_err(err);
>> +        error_propagate(errp, err);
>>          object_unparent(OBJECT(dev));
>>          object_unref(OBJECT(dev));
>>          return NULL;
>> @@ -601,12 +595,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>      dev->opts = opts;
>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>      if (err != NULL) {
>> -        qerror_report_err(err);
>> -        error_free(err);
>> +        error_propagate(errp, err);
>>          dev->opts = NULL;
>>          object_unparent(OBJECT(dev));
>>          object_unref(OBJECT(dev));
>> -        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>          return NULL;
>>      }
>>      return dev;
>> @@ -779,8 +771,10 @@ int do_device_add(Monitor *mon, const QDict *qdict, 
>> QObject **ret_data)
>>          qemu_opts_del(opts);
>>          return 0;
>>      }
>> -    dev = qdev_device_add(opts);
>> +    dev = qdev_device_add(opts, &local_err);
>>      if (!dev) {
>
> Would seem safer to change this to "if (local_err) {" now that it is
> dealing with local_err.
>
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
>>          qemu_opts_del(opts);
>>          return -1;
>>      }

After my next patch series, this will be

    dev = qdev_device_add(opts, &local_err);
    if (!dev) {
        error_propagate(errp, local_err);
        qemu_opts_del(opts);
        return;
    }

We got similar logic all over the tree.

Whether to check the returned pointer or the error variable is a matter
of taste.  I prefer to check the pointer, not least because it's less
likely to confuse Coverity in my experience.

>> diff --git a/vl.c b/vl.c
>> index 9542095..f0dcb6b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2185,11 +2185,14 @@ static int device_help_func(void *opaque, QemuOpts 
>> *opts, Error **errp)
>>  
>>  static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
>>  {
>> +    Error *err = NULL;
>>      DeviceState *dev;
>>  
>> -    dev = qdev_device_add(opts);
>> -    if (!dev)
>> +    dev = qdev_device_add(opts, &err);
>> +    if (!dev) {
>> +        error_report_err(err);
>>          return -1;
>> +    }
>>      object_unref(OBJECT(dev));
>>      return 0;
>>  }
>
> Anyway,
>
> Reviewed-by: Andreas Färber <address@hidden>

Thanks!