On 02/04/13 18:51, Luiz Capitulino wrote:
> On Fri, 1 Feb 2013 18:38:18 +0100
> Laszlo Ersek <ler...@redhat.com> wrote:
>
>>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>
> Splitting this as I suggested in the other patch would make review
> easier. I honestly got a bit lost while reviewing this one.
I think you cannot review this series without applying each patch in
lock-step with the review, and contrasting the next patch with the
just-updated tree. In that light it's especially regrettable that my
series didn't apply to master (it certainly did when I posted it).
Anyway the approach I took was:
- determine the set of functions to modify,
- work in a bottom-up fashion,
- when changing a function signature, change all callers.
Suppose there are three functions, X, Y, Z, with calls like X->Y, X->Z.
#1 First I'd fix Y (signature change) and adapt its call site in X --
acccept the error and print it. At this point X consumes a few errors,
and some other errors don't reach it at all.
#2 Then I'd fix Z (signature change) and adapt its call site in X --
accept the error and print it. At this point Y and Z are OK, and X has
several places that accept and consume (print/free) errors.
#3 In the next patch, X's signature is changed, all error consumption /
printing sites in X are changed to propagate / generate instead, and all
call sites or X are updated.
The only patch I could split is #3, but then X would in part consume, in
part propagate errors. Do you suggest I do that?
Thanks!
Laszlo
>
>> ---
>> hw/qdev-monitor.c | 29 +++++++++++++++--------------
>> 1 files changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 83540fc..0c01b04 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -324,7 +324,7 @@ static BusState *qbus_find_recursive(BusState *bus,
>> const char *name,
>> return NULL;
>> }
>>
>> -static BusState *qbus_find(const char *path)
>> +static BusState *qbus_find(const char *path, Error **errp)
>> {
>> DeviceState *dev;
>> BusState *bus;
>> @@ -336,20 +336,19 @@ static BusState *qbus_find(const char *path)
>> bus = sysbus_get_default();
>> pos = 0;
>> } else {
>> - Error *err = NULL;
>> + Error *find_err = NULL;
>>
>> if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
>> assert(!path[0]);
>> elem[0] = len = 0;
>> }
>> - bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
>> + bus = qbus_find_recursive(sysbus_get_default(), elem, NULL,
>> &find_err);
>> if (!bus) {
>> - if (error_is_set(&err)) {
>> - qerror_report_err(err);
>> - error_free(err);
>> - } else {
>> - qerror_report(QERR_BUS_NOT_FOUND, elem);
>> - }
>> + Error *not_found;
>> +
>> + error_set(¬_found, QERR_BUS_NOT_FOUND, elem);
>> + error_propagate(errp, find_err);
>> + error_propagate(errp, not_found);
>> return NULL;
>> }
>> pos = len;
>> @@ -372,7 +371,7 @@ static BusState *qbus_find(const char *path)
>> pos += len;
>> dev = qbus_find_dev(bus, elem);
>> if (!dev) {
>> - qerror_report(QERR_DEVICE_NOT_FOUND, elem);
>> + error_set(errp, QERR_DEVICE_NOT_FOUND, elem);
>> if (!monitor_cur_is_qmp()) {
>> qbus_list_dev(bus);
>> }
>> @@ -388,12 +387,12 @@ static BusState *qbus_find(const char *path)
>> * one child bus accept it nevertheless */
>> switch (dev->num_child_bus) {
>> case 0:
>> - qerror_report(QERR_DEVICE_NO_BUS, elem);
>> + error_set(errp, QERR_DEVICE_NO_BUS, elem);
>> return NULL;
>> case 1:
>> return QLIST_FIRST(&dev->child_bus);
>> default:
>> - qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
>> + error_set(errp, QERR_DEVICE_MULTIPLE_BUSSES, elem);
>> if (!monitor_cur_is_qmp()) {
>> qbus_list_bus(dev);
>> }
>> @@ -409,7 +408,7 @@ static BusState *qbus_find(const char *path)
>> pos += len;
>> bus = qbus_find_bus(dev, elem);
>> if (!bus) {
>> - qerror_report(QERR_BUS_NOT_FOUND, elem);
>> + error_set(errp, QERR_BUS_NOT_FOUND, elem);
>> if (!monitor_cur_is_qmp()) {
>> qbus_list_bus(dev);
>> }
>> @@ -454,8 +453,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>> /* find bus */
>> path = qemu_opt_get(opts, "bus");
>> if (path != NULL) {
>> - bus = qbus_find(path);
>> + bus = qbus_find(path, &local_err);
>> if (!bus) {
>> + qerror_report_err(local_err);
>> + error_free(local_err);
>> return NULL;
>> }
>> if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {
>
