| Message ID | 1359740299-27968-7-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 1 Feb 2013 18:38:18 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > > Signed-off-by: Laszlo Ersek <lersek@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. > --- > 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)) {
On 02/04/13 18:51, Luiz Capitulino wrote: > On Fri, 1 Feb 2013 18:38:18 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> >> Signed-off-by: Laszlo Ersek <lersek@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)) { >
On Mon, 04 Feb 2013 19:23:15 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/04/13 18:51, Luiz Capitulino wrote: > > On Fri, 1 Feb 2013 18:38:18 +0100 > > Laszlo Ersek <lersek@redhat.com> wrote: > > > >> > >> Signed-off-by: Laszlo Ersek <lersek@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). I'll review it in more detail when you respin. > > 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. May work for simple cases (although has the minor problem of mixing signature change with adding new error handling to X), but doesn't scale when Y is called by more than one function and their error handling differ. > > #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? The suggestion I gave in another email is to separate signature changes from error handling changes. Taking your example above, I'd have the following patches: 1. Change Y's signature to take an Error ** argument and change X to pass NULL for the new argument 2. Change Z's signature to take an Error ** argument and change X to pass NULL for the new argument 3. Change X to pass an non-NULL Error ** argument to Y and Z and handle error accordingly 4. If Y is non-void and if the return value is success/failure, drop it 5. If Y is non-void and if the return value is success/failure, drop it Now, you could merge patches 1 and 2 if the functions are only used by X. On the other hand, if Y and/or Z are called by other functions, you'd have to add more patches after 3 to add error handling to them. I know this is more work, but it makes it easier to review.
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)) {
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-monitor.c | 29 +++++++++++++++-------------- 1 files changed, 15 insertions(+), 14 deletions(-)