Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()
On Fri, Dec 11, 2015 at 10:49:40AM +0100, Thomas Huth wrote: > On 11/12/15 01:11, David Gibson wrote: > > Use error_setg() to return an error instead of using an explicit exit(). > > > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0ff09b9..fd16db4 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState > > *spapr, Error **errp) > > > > static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque) > > { > > +Error **errp = opaque; > > bool matched = false; > > > > if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) { > > @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice > > *sbdev, void *opaque) > > } > > > > if (!matched) { > > -error_report("Device %s is not supported by this machine yet.", > > - qdev_fw_name(DEVICE(sbdev))); > > -exit(1); > > +error_setg(errp, > > + "Device %s is not supported by this machine yet", > > + qdev_fw_name(DEVICE(sbdev))); > > } > > > > return 0; > > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void) > > uint32_t rtas_limit; > > > > /* Check for unknown sysbus devices */ > > -foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > > +foreach_dynamic_sysbus_device(find_unknown_sysbus_device, > > &error_abort); > > > > /* Reset the hash table & recalc the RMA */ > > spapr_reset_htab(spapr, &error_abort); > > Can this error be triggered by the user (by plugging certain devices or > so)? If so, I'm not sure whether error_abort is the right thing to use > here ... it's always ugly if the user can trigger an abort directly. > Maybe error_fatal would be better instead? Hm. So, yes, I think it can be triggered by the user. I used &error_abort on the grounds that this is during reset - after the initial setup phase. Markus, do you have an opinion on what the right handling here is? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()
On Fri, Dec 11, 2015 at 08:15:39AM -0700, Eric Blake wrote: > On 12/10/2015 05:11 PM, David Gibson wrote: > > Use error_setg() to return an error instead of using an explicit exit(). > > > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > +++ b/hw/ppc/spapr.c > > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState > > *spapr, Error **errp) > > > > static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque) > > { > > > if (!matched) { > > -error_report("Device %s is not supported by this machine yet.", > > - qdev_fw_name(DEVICE(sbdev))); > > -exit(1); > > +error_setg(errp, > > + "Device %s is not supported by this machine yet", > > + qdev_fw_name(DEVICE(sbdev))); > > } > > > > return 0; > > It looks like find_unknown_sysbus_device is designed to be called in a > loop, and that returning 0 lets the loop continue. > > > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void) > > uint32_t rtas_limit; > > > > /* Check for unknown sysbus devices */ > > -foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > > +foreach_dynamic_sysbus_device(find_unknown_sysbus_device, > > &error_abort); > > If a caller passes something other than &error_abort as the opaque, AND > the error condition occurs more than once in the loop iteration, then > you really need to change find_unknown_sysbus_device() to return > non-zero after raising error on the first failure, so that the loop > doesn't continue on to a second pass and attempt an error_setg() onto an > already-set error. Ah, good catch. I'll correct in the next spin. > Of course, since _this_ patch uses &error_abort as the only client, the > loop will never continue after the first failure, but it's more robust > to be clean without having to audit the callers. > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()
On 12/10/2015 05:11 PM, David Gibson wrote: > Use error_setg() to return an error instead of using an explicit exit(). > > Signed-off-by: David Gibson > --- > hw/ppc/spapr.c | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > +++ b/hw/ppc/spapr.c > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, > Error **errp) > > static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque) > { > if (!matched) { > -error_report("Device %s is not supported by this machine yet.", > - qdev_fw_name(DEVICE(sbdev))); > -exit(1); > +error_setg(errp, > + "Device %s is not supported by this machine yet", > + qdev_fw_name(DEVICE(sbdev))); > } > > return 0; It looks like find_unknown_sysbus_device is designed to be called in a loop, and that returning 0 lets the loop continue. > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void) > uint32_t rtas_limit; > > /* Check for unknown sysbus devices */ > -foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > +foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort); If a caller passes something other than &error_abort as the opaque, AND the error condition occurs more than once in the loop iteration, then you really need to change find_unknown_sysbus_device() to return non-zero after raising error on the first failure, so that the loop doesn't continue on to a second pass and attempt an error_setg() onto an already-set error. Of course, since _this_ patch uses &error_abort as the only client, the loop will never continue after the first failure, but it's more robust to be clean without having to audit the callers. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()
On 11/12/15 01:11, David Gibson wrote: > Use error_setg() to return an error instead of using an explicit exit(). > > Signed-off-by: David Gibson > --- > hw/ppc/spapr.c | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0ff09b9..fd16db4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, > Error **errp) > > static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque) > { > +Error **errp = opaque; > bool matched = false; > > if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) { > @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice > *sbdev, void *opaque) > } > > if (!matched) { > -error_report("Device %s is not supported by this machine yet.", > - qdev_fw_name(DEVICE(sbdev))); > -exit(1); > +error_setg(errp, > + "Device %s is not supported by this machine yet", > + qdev_fw_name(DEVICE(sbdev))); > } > > return 0; > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void) > uint32_t rtas_limit; > > /* Check for unknown sysbus devices */ > -foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > +foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort); > > /* Reset the hash table & recalc the RMA */ > spapr_reset_htab(spapr, &error_abort); Can this error be triggered by the user (by plugging certain devices or so)? If so, I'm not sure whether error_abort is the right thing to use here ... it's always ugly if the user can trigger an abort directly. Maybe error_fatal would be better instead? Thomas