| Message ID | 20200914123505.612812-15-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 2) | expand |
14.09.2020 15:35, Greg Kurz wrote: > As recommended in "qapi/error.h", add a bool return value to > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > of local_err in spapr_memory_plug(). > > Since object_property_get_uint() only returns 0 on failure, use > that as well. Why are you sure? Can't the property be 0 itself? > > Also call ERRP_GUARD() to be able to check the status of void > function pc_dimm_plug() with *errp. >
On Tue, 15 Sep 2020 13:58:53 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 14.09.2020 15:35, Greg Kurz wrote: > > As recommended in "qapi/error.h", add a bool return value to > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > > of local_err in spapr_memory_plug(). > > > > Since object_property_get_uint() only returns 0 on failure, use > > that as well. > > Why are you sure? Can't the property be 0 itself? > Hmmm... I've based this assumption on the header: * Returns: the value of the property, converted to an unsigned integer, or 0 * an error occurs (including when the property value is not an integer). but looking at the implementation, I don't see any check that a property cannot be 0 indeed... It's a bit misleading to mention this in the header though. I understand that the function should return something, which should have a some explicitly assigned value to avoid compilers or static analyzers to yell, but the written contract should be that the value is _undefined_ IMHO. In its present form, the only way to know if the property is valid is to pass a non-NULL errp actually. I'd rather even see that in the contract, and an assert() in the code. An alternative would be to convert it to have a bool return value and get the actual uint result with a pointer argument. > > > > Also call ERRP_GUARD() to be able to check the status of void > > function pc_dimm_plug() with *errp. > > > > I'm now hesitating to either check *errp for object_property_get_uint() too or simply drop this patch...
15.09.2020 14:43, Greg Kurz wrote: > On Tue, 15 Sep 2020 13:58:53 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 14.09.2020 15:35, Greg Kurz wrote: >>> As recommended in "qapi/error.h", add a bool return value to >>> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead >>> of local_err in spapr_memory_plug(). >>> >>> Since object_property_get_uint() only returns 0 on failure, use >>> that as well. >> >> Why are you sure? Can't the property be 0 itself? >> > > Hmmm... I've based this assumption on the header: > > * Returns: the value of the property, converted to an unsigned integer, or 0 > * an error occurs (including when the property value is not an integer). > > but looking at the implementation, I don't see any check that > a property cannot be 0 indeed... > > It's a bit misleading to mention this in the header though. I > understand that the function should return something, which > should have a some explicitly assigned value to avoid compilers > or static analyzers to yell, but the written contract should be > that the value is _undefined_ IMHO. > > In its present form, the only way to know if the property is > valid is to pass a non-NULL errp actually. I'd rather even see > that in the contract, and an assert() in the code. > > An alternative would be to convert it to have a bool return > value and get the actual uint result with a pointer argument. > >>> >>> Also call ERRP_GUARD() to be able to check the status of void >>> function pc_dimm_plug() with *errp. >>> >> >> > > I'm now hesitating to either check *errp for object_property_get_uint() > too or simply drop this patch... > .. and the following. If no more reviewers come to look at it, you can just merge first 13 patches, not resending the whole series for last two patches, which may be moved to round 3.
I've dropped the SPAM mentions from the subject this time :) On Tue, 15 Sep 2020 14:53:53 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 15.09.2020 14:43, Greg Kurz wrote: > > On Tue, 15 Sep 2020 13:58:53 +0300 > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > >> 14.09.2020 15:35, Greg Kurz wrote: > >>> As recommended in "qapi/error.h", add a bool return value to > >>> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > >>> of local_err in spapr_memory_plug(). > >>> > >>> Since object_property_get_uint() only returns 0 on failure, use > >>> that as well. > >> > >> Why are you sure? Can't the property be 0 itself? > >> > > > > Hmmm... I've based this assumption on the header: > > > > * Returns: the value of the property, converted to an unsigned integer, or 0 > > * an error occurs (including when the property value is not an integer). > > > > but looking at the implementation, I don't see any check that > > a property cannot be 0 indeed... > > > > It's a bit misleading to mention this in the header though. I > > understand that the function should return something, which > > should have a some explicitly assigned value to avoid compilers > > or static analyzers to yell, but the written contract should be > > that the value is _undefined_ IMHO. > > > > In its present form, the only way to know if the property is > > valid is to pass a non-NULL errp actually. I'd rather even see > > that in the contract, and an assert() in the code. > > > > An alternative would be to convert it to have a bool return > > value and get the actual uint result with a pointer argument. > > > >>> > >>> Also call ERRP_GUARD() to be able to check the status of void > >>> function pc_dimm_plug() with *errp. > >>> > >> > >> > > > > I'm now hesitating to either check *errp for object_property_get_uint() > > too or simply drop this patch... > > > > .. and the following. If no more reviewers come to look at it, you can just merge first 13 patches, not resending the whole series for last two patches, which may be moved to round 3. > I don't expect much people except David or maybe Markus to look at these patches actually. And anyway, it's up to David to merge them. But, yes, I agree patch 14 and 15 should go to the next round. Thanks for the review ! Cheers, -- Greg
On Tue, 15 Sep 2020 14:04:23 +0200 Greg Kurz <groug@kaod.org> wrote: > > I don't expect much people except David or maybe Markus to look > at these patches actually. And anyway, it's up to David to merge My bad, I didn't think about Philippe... :P Thanks for the review, Philippe ! Cheers, -- Greg > them. But, yes, I agree patch 14 and 15 should go to the next > round. > > Thanks for the review ! > > Cheers, > > -- > Greg >
On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote: > On Tue, 15 Sep 2020 13:58:53 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > 14.09.2020 15:35, Greg Kurz wrote: > > > As recommended in "qapi/error.h", add a bool return value to > > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > > > of local_err in spapr_memory_plug(). > > > > > > Since object_property_get_uint() only returns 0 on failure, use > > > that as well. > > > > Why are you sure? Can't the property be 0 itself? > > > > Hmmm... I've based this assumption on the header: > > * Returns: the value of the property, converted to an unsigned integer, or 0 > * an error occurs (including when the property value is not an integer). > > but looking at the implementation, I don't see any check that > a property cannot be 0 indeed... Yeah, indeed I'm pretty sure it can. > It's a bit misleading to mention this in the header though. I > understand that the function should return something, which > should have a some explicitly assigned value to avoid compilers > or static analyzers to yell, but the written contract should be > that the value is _undefined_ IMHO. Hrm... I think the description could be clearer, but returning 0 explicitly on the failure case has some benefits too. If 0 is a reasonable default for when the property isn't present (which is a plausibly common case) then it means you can just get a value and ignore errors. > In its present form, the only way to know if the property is > valid is to pass a non-NULL errp actually. I'd rather even see > that in the contract, and an assert() in the code. Maybe... see above. > An alternative would be to convert it to have a bool return > value and get the actual uint result with a pointer argument. I don't think this is a good idea. Returning success/failure as the return value is a good rule of thumb because it reduces the amount of checking of out-of-band information you need to do. If you move to returning the actual value you're trying to get out of band in this sense, it kind of defeats that purpose. I think this one is a case where it is reasonable to make it required to explicitly check the error value. > > > Also call ERRP_GUARD() to be able to check the status of void > > > function pc_dimm_plug() with *errp. > > I'm now hesitating to either check *errp for object_property_get_uint() > too or simply drop this patch...
David Gibson <david@gibson.dropbear.id.au> writes: > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote: >> On Tue, 15 Sep 2020 13:58:53 +0300 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >> > 14.09.2020 15:35, Greg Kurz wrote: >> > > As recommended in "qapi/error.h", add a bool return value to >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead >> > > of local_err in spapr_memory_plug(). >> > > >> > > Since object_property_get_uint() only returns 0 on failure, use >> > > that as well. >> > >> > Why are you sure? Can't the property be 0 itself? >> > >> >> Hmmm... I've based this assumption on the header: >> >> * Returns: the value of the property, converted to an unsigned integer, or 0 >> * an error occurs (including when the property value is not an integer). >> >> but looking at the implementation, I don't see any check that >> a property cannot be 0 indeed... > > Yeah, indeed I'm pretty sure it can. Correct. Corollary: you can't use to return value to check for failure, except when you know the property cannot be zero (you commonly don't). The function predates our (rather late) realization that returning a recognizable error value in addition to setting an error leads to more readable code. Today, we'd perhaps do it the way you describe below. >> It's a bit misleading to mention this in the header though. I >> understand that the function should return something, which >> should have a some explicitly assigned value to avoid compilers >> or static analyzers to yell, but the written contract should be >> that the value is _undefined_ IMHO. > > Hrm... I think the description could be clearer, but returning 0 > explicitly on the failure case has some benefits too. If 0 is a > reasonable default for when the property isn't present (which is a > plausibly common case) then it means you can just get a value and > ignore errors. Matter of taste. There's no shortage of _undefined_ in C... >> In its present form, the only way to know if the property is >> valid is to pass a non-NULL errp actually. I'd rather even see >> that in the contract, and an assert() in the code. > > Maybe... see above. If you think the contract could be improved, please post a patch. What assertion do you have in mind? If it's adding assert(errp) to object_property_get_uint(), I'll object. Functions should not place additional restrictions on @errp arguments without a compelling reason. >> An alternative would be to convert it to have a bool return >> value and get the actual uint result with a pointer argument. > > I don't think this is a good idea. Returning success/failure as the > return value is a good rule of thumb because it reduces the amount of > checking of out-of-band information you need to do. If you move to > returning the actual value you're trying to get out of band in this > sense, it kind of defeats that purpose. > > I think this one is a case where it is reasonable to make it required > to explicitly check the error value. If almost all calls assign the value to a variable, like val = object_property_get_uint(obj, name, &err); if (err) { error_propagate(errp, err) ... bail out ... } ... use val ... then the alternative Greg proposed is easier on the eyes: if (!object_property_get_uint(obj, name, &val, errp)) { ... bail out ... } ... use val ... It isn't for calls that use the value without storing it in a variable first. >> > > Also call ERRP_GUARD() to be able to check the status of void >> > > function pc_dimm_plug() with *errp. >> >> I'm now hesitating to either check *errp for object_property_get_uint() >> too or simply drop this patch...
On Thu, 17 Sep 2020 09:38:49 +0200 Markus Armbruster <armbru@redhat.com> wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote: > >> On Tue, 15 Sep 2020 13:58:53 +0300 > >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> > >> > 14.09.2020 15:35, Greg Kurz wrote: > >> > > As recommended in "qapi/error.h", add a bool return value to > >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > >> > > of local_err in spapr_memory_plug(). > >> > > > >> > > Since object_property_get_uint() only returns 0 on failure, use > >> > > that as well. > >> > > >> > Why are you sure? Can't the property be 0 itself? > >> > > >> > >> Hmmm... I've based this assumption on the header: > >> > >> * Returns: the value of the property, converted to an unsigned integer, or 0 > >> * an error occurs (including when the property value is not an integer). > >> > >> but looking at the implementation, I don't see any check that > >> a property cannot be 0 indeed... > > > > Yeah, indeed I'm pretty sure it can. > > Correct. > > Corollary: you can't use to return value to check for failure, except > when you know the property cannot be zero (you commonly don't). > > The function predates our (rather late) realization that returning a > recognizable error value in addition to setting an error leads to more > readable code. Today, we'd perhaps do it the way you describe below. > > >> It's a bit misleading to mention this in the header though. I > >> understand that the function should return something, which > >> should have a some explicitly assigned value to avoid compilers > >> or static analyzers to yell, but the written contract should be > >> that the value is _undefined_ IMHO. > > > > Hrm... I think the description could be clearer, but returning 0 > > explicitly on the failure case has some benefits too. If 0 is a > > reasonable default for when the property isn't present (which is a > > plausibly common case) then it means you can just get a value and > > ignore errors. > > Matter of taste. > > There's no shortage of _undefined_ in C... > Yeah and each compiler has its take as how to handle that. FWIW see section 3.1 of this bachelor thesis on the topic: https://www.cs.ru.nl/bachelors-theses/2017/Matthias_Vogelaar___4372913___Defining_the_Undefined_in_C.pdf > >> In its present form, the only way to know if the property is > >> valid is to pass a non-NULL errp actually. I'd rather even see > >> that in the contract, and an assert() in the code. > > > > Maybe... see above. > > If you think the contract could be improved, please post a patch. > The contract of object_property_get_enum() which is the next function in object.h explicitly says that the result is undefined, even if the implementation returns 0. So I was thinking of doing the same for object_property_get_uint(). > What assertion do you have in mind? If it's adding assert(errp) to > object_property_get_uint(), I'll object. Functions should not place > additional restrictions on @errp arguments without a compelling reason. > I had such an assertion in mind but if you think this restriction is abusive, I take your word :) > >> An alternative would be to convert it to have a bool return > >> value and get the actual uint result with a pointer argument. > > > > I don't think this is a good idea. Returning success/failure as the > > return value is a good rule of thumb because it reduces the amount of > > checking of out-of-band information you need to do. If you move to > > returning the actual value you're trying to get out of band in this > > sense, it kind of defeats that purpose. > > > > I think this one is a case where it is reasonable to make it required > > to explicitly check the error value. > > If almost all calls assign the value to a variable, like > > val = object_property_get_uint(obj, name, &err); > if (err) { > error_propagate(errp, err) > ... bail out ... > } > ... use val ... > > then the alternative Greg proposed is easier on the eyes: > > if (!object_property_get_uint(obj, name, &val, errp)) { > ... bail out ... > } > ... use val ... > That's what I had in mind. > It isn't for calls that use the value without storing it in a variable > first. > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l 60 Manual inspecting the output of $ git grep -W object_property_get_uint -- :^{include,qom/object.c} ... seems to be showing that most users simply ignore errors (ie. pass NULL as 3rd argument). Then some users pass &error_abort and only a few pass a &err or errp. Assuming that users know what they're doing, I guess my proposal wouldn't bring much to the code base in the end... I'm not even sure now that it's worth changing the contract. Cheers, -- Greg > >> > > Also call ERRP_GUARD() to be able to check the status of void > >> > > function pc_dimm_plug() with *errp. > >> > >> I'm now hesitating to either check *errp for object_property_get_uint() > >> too or simply drop this patch... >
Greg Kurz <groug@kaod.org> writes: > On Thu, 17 Sep 2020 09:38:49 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> David Gibson <david@gibson.dropbear.id.au> writes: >> >> > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote: >> >> On Tue, 15 Sep 2020 13:58:53 +0300 >> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >> >> >> > 14.09.2020 15:35, Greg Kurz wrote: >> >> > > As recommended in "qapi/error.h", add a bool return value to >> >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead >> >> > > of local_err in spapr_memory_plug(). >> >> > > >> >> > > Since object_property_get_uint() only returns 0 on failure, use >> >> > > that as well. >> >> > >> >> > Why are you sure? Can't the property be 0 itself? >> >> > >> >> >> >> Hmmm... I've based this assumption on the header: >> >> >> >> * Returns: the value of the property, converted to an unsigned integer, or 0 >> >> * an error occurs (including when the property value is not an integer). >> >> >> >> but looking at the implementation, I don't see any check that >> >> a property cannot be 0 indeed... >> > >> > Yeah, indeed I'm pretty sure it can. >> >> Correct. >> >> Corollary: you can't use to return value to check for failure, except >> when you know the property cannot be zero (you commonly don't). >> >> The function predates our (rather late) realization that returning a >> recognizable error value in addition to setting an error leads to more >> readable code. Today, we'd perhaps do it the way you describe below. >> >> >> It's a bit misleading to mention this in the header though. I >> >> understand that the function should return something, which >> >> should have a some explicitly assigned value to avoid compilers >> >> or static analyzers to yell, but the written contract should be >> >> that the value is _undefined_ IMHO. >> > >> > Hrm... I think the description could be clearer, but returning 0 >> > explicitly on the failure case has some benefits too. If 0 is a >> > reasonable default for when the property isn't present (which is a >> > plausibly common case) then it means you can just get a value and >> > ignore errors. >> >> Matter of taste. >> >> There's no shortage of _undefined_ in C... >> > > Yeah and each compiler has its take as how to handle that. > > FWIW see section 3.1 of this bachelor thesis on the topic: > > https://www.cs.ru.nl/bachelors-theses/2017/Matthias_Vogelaar___4372913___Defining_the_Undefined_in_C.pdf > >> >> In its present form, the only way to know if the property is >> >> valid is to pass a non-NULL errp actually. I'd rather even see >> >> that in the contract, and an assert() in the code. >> > >> > Maybe... see above. >> >> If you think the contract could be improved, please post a patch. >> > > The contract of object_property_get_enum() which is the next function > in object.h explicitly says that the result is undefined, even if > the implementation returns 0. So I was thinking of doing the same > for object_property_get_uint(). Let's survey actual behavior of the object_property_get*(): return value function on success on error o_p_get() true false o_p_get_str() non-null null o_p_get_link() anything null o_p_get_bool() anything false o_p_get_int() anything -1 o_p_get_uint() anything 0 o_p_get_enum() enum value 0 or -1 object_property_get() and object_property_get_str() have a distinct error value. Yes, a QAPI str cannot be null. object_property_get_enum() has *two* error values, and one of them can also occur as success value. This is daft. I'll send a patch to always return -1 on error. Bonus: distinct error value. object_property_get_link(), _bool(), _int(), and _uint() don't have a distinct error value. >> What assertion do you have in mind? If it's adding assert(errp) to >> object_property_get_uint(), I'll object. Functions should not place >> additional restrictions on @errp arguments without a compelling reason. >> > > I had such an assertion in mind but if you think this restriction is > abusive, I take your word :) > >> >> An alternative would be to convert it to have a bool return >> >> value and get the actual uint result with a pointer argument. >> > >> > I don't think this is a good idea. Returning success/failure as the >> > return value is a good rule of thumb because it reduces the amount of >> > checking of out-of-band information you need to do. If you move to >> > returning the actual value you're trying to get out of band in this >> > sense, it kind of defeats that purpose. >> > >> > I think this one is a case where it is reasonable to make it required >> > to explicitly check the error value. >> >> If almost all calls assign the value to a variable, like >> >> val = object_property_get_uint(obj, name, &err); >> if (err) { >> error_propagate(errp, err) >> ... bail out ... >> } >> ... use val ... >> >> then the alternative Greg proposed is easier on the eyes: >> >> if (!object_property_get_uint(obj, name, &val, errp)) { >> ... bail out ... >> } >> ... use val ... >> > > That's what I had in mind. > >> It isn't for calls that use the value without storing it in a variable >> first. >> > > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l > 60 > > Manual inspecting the output of > > $ git grep -W object_property_get_uint -- :^{include,qom/object.c} > ... > > seems to be showing that most users simply ignore errors (ie. pass NULL > as 3rd argument). Then some users pass &error_abort and only a few > pass a &err or errp. > > Assuming that users know what they're doing, I guess my proposal > wouldn't bring much to the code base in the end... I'm not even > sure now that it's worth changing the contract. We'd change val = object_property_get_uint(obj, name, &error_abort); to object_property_get_uint(obj, name, &val, &error_abort); which is not an improvement. Most of the ones passing NULL should probably pass &error_abort instead. Doesn't change the argument. > Cheers, > > -- > Greg > >> >> > > Also call ERRP_GUARD() to be able to check the status of void >> >> > > function pc_dimm_plug() with *errp. >> >> >> >> I'm now hesitating to either check *errp for object_property_get_uint() >> >> too or simply drop this patch... >>
On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote: > Greg Kurz <groug@kaod.org> writes: > > > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l > > 60 > > > > Manual inspecting the output of > > > > $ git grep -W object_property_get_uint -- :^{include,qom/object.c} > > ... > > > > seems to be showing that most users simply ignore errors (ie. pass NULL > > as 3rd argument). Then some users pass &error_abort and only a few > > pass a &err or errp. > > > > Assuming that users know what they're doing, I guess my proposal > > wouldn't bring much to the code base in the end... I'm not even > > sure now that it's worth changing the contract. > > We'd change > > val = object_property_get_uint(obj, name, &error_abort); > > to > > object_property_get_uint(obj, name, &val, &error_abort); > > which is not an improvement. > > Most of the ones passing NULL should probably pass &error_abort > instead. Doesn't change the argument. I wonder if we actually need to have an Error parameter at all for the getters. IIUC the only valid runtime error is probably the case where the property has not been set, and even then, I think properties usually have a default value that would be returned. All the other errors look like programmer errors. IOW, can we remove the Error parameter and have all the o_p_get* method impls use error_abort. In the rare case where a caller needs to handle a property not being set, they can use object_property_find() as a test before invoking the getter. Of course requires someone motivated to audit all the case that are not using NULL or error_abort and decide whether the attempt at passing a local errp was actually justified or not. Regards, Daniel
On Thu, 17 Sep 2020 13:18:51 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote: > > Greg Kurz <groug@kaod.org> writes: > > > > > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l > > > 60 > > > > > > Manual inspecting the output of > > > > > > $ git grep -W object_property_get_uint -- :^{include,qom/object.c} > > > ... > > > > > > seems to be showing that most users simply ignore errors (ie. pass NULL > > > as 3rd argument). Then some users pass &error_abort and only a few > > > pass a &err or errp. > > > > > > Assuming that users know what they're doing, I guess my proposal > > > wouldn't bring much to the code base in the end... I'm not even > > > sure now that it's worth changing the contract. > > > > We'd change > > > > val = object_property_get_uint(obj, name, &error_abort); > > > > to > > > > object_property_get_uint(obj, name, &val, &error_abort); > > > > which is not an improvement. > > > > Most of the ones passing NULL should probably pass &error_abort > > instead. Doesn't change the argument. > > I wonder if we actually need to have an Error parameter at all for > the getters. IIUC the only valid runtime error is probably the case > where the property has not been set, and even then, I think properties > usually have a default value that would be returned. All the other > errors look like programmer errors. > > IOW, can we remove the Error parameter and have all the o_p_get* > method impls use error_abort. > > In the rare case where a caller needs to handle a property not > being set, they can use object_property_find() as a test before > invoking the getter. > I tend to agree. > Of course requires someone motivated to audit all the case that > are not using NULL or error_abort and decide whether the attempt > at passing a local errp was actually justified or not. > Since I've open the can of worms, I'm volunteering to do that if we have a consensus on the fact that "the only valid runtime error os the case the property has not been set". Cheers, -- Greg > Regards, > Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote: >> Greg Kurz <groug@kaod.org> writes: >> >> > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l >> > 60 >> > >> > Manual inspecting the output of >> > >> > $ git grep -W object_property_get_uint -- :^{include,qom/object.c} >> > ... >> > >> > seems to be showing that most users simply ignore errors (ie. pass NULL >> > as 3rd argument). Then some users pass &error_abort and only a few >> > pass a &err or errp. >> > >> > Assuming that users know what they're doing, I guess my proposal >> > wouldn't bring much to the code base in the end... I'm not even >> > sure now that it's worth changing the contract. >> >> We'd change >> >> val = object_property_get_uint(obj, name, &error_abort); >> >> to >> >> object_property_get_uint(obj, name, &val, &error_abort); >> >> which is not an improvement. >> >> Most of the ones passing NULL should probably pass &error_abort >> instead. Doesn't change the argument. > > I wonder if we actually need to have an Error parameter at all for > the getters. IIUC the only valid runtime error is probably the case > where the property has not been set, and even then, I think properties > usually have a default value that would be returned. All the other > errors look like programmer errors. From the notes I took when I last hacked a trail through this jungle... Failure modes of object_property_get() @name not found @name permission, i.e. no ->get ->get() fails (how?) object_property_get_{str,bool,int,uint}() object_property_get_qobject() object_property_get() with qobject output visitor which see prop value qobject not a string / bool / int / uint object_property_get_enum() @name not found @typename doesn't match object_property_get() with string output visitor which see qapi_enum_parse() prop value not a value of enum @typename object_property_get_link() object_property_get_str() which see prop value does not resolve I think most of these failures are obviously programming errors most of the time. Many callers treat failures as programming errors by passing &error_abort. Many callers ignore errors by passing NULL. I believe most of them should really pass &error_abort instead. Fixing them is tedious, because you have to check what would happen on error. If the answer is "chaos", fix to pass &error_abort. But the answer can also be "works as intended", or "beats me". > IOW, can we remove the Error parameter and have all the o_p_get* > method impls use error_abort. If we fix the callers that should pass &error_abort to do so, we'll see what remains, and whether we can drop the parameter. > In the rare case where a caller needs to handle a property not > being set, they can use object_property_find() as a test before > invoking the getter. I dislike "if (X() is going to fail) do something else; else X()". I guess it could be okay for the narrow case of "property does not exist". > Of course requires someone motivated to audit all the case that > are not using NULL or error_abort and decide whether the attempt > at passing a local errp was actually justified or not. We got one! Thanks, Greg :)
diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h index b834d82f5545..dc30edece997 100644 --- a/include/hw/ppc/spapr_nvdimm.h +++ b/include/hw/ppc/spapr_nvdimm.h @@ -30,7 +30,7 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, uint64_t size, Error **errp); -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr); #endif diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index e813c7cfb949..d6b4de6a1c53 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3366,7 +3366,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, return 0; } -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, bool dedicated_hp_event_source, Error **errp) { SpaprDrc *drc; @@ -3387,7 +3387,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, addr / SPAPR_MEMORY_BLOCK_SIZE); spapr_drc_detach(drc); } - return; + return false; } if (!hotplugged) { spapr_drc_reset(drc); @@ -3409,12 +3409,13 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, nr_lmbs); } } + return true; } static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - Error *local_err = NULL; + ERRP_GUARD(); SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); uint64_t size, addr, slot; @@ -3422,39 +3423,29 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort); - pc_dimm_plug(dimm, MACHINE(ms), &local_err); - if (local_err) { - goto out; + pc_dimm_plug(dimm, MACHINE(ms), errp); + if (*errp) { + return; } if (!is_nvdimm) { - addr = object_property_get_uint(OBJECT(dimm), - PC_DIMM_ADDR_PROP, &local_err); - if (local_err) { + addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, errp); + if (!addr || + !spapr_add_lmbs(dev, addr, size, + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) { goto out_unplug; } - spapr_add_lmbs(dev, addr, size, - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), - &local_err); } else { - slot = object_property_get_uint(OBJECT(dimm), - PC_DIMM_SLOT_PROP, &local_err); - if (local_err) { + slot = object_property_get_uint(OBJECT(dimm), PC_DIMM_SLOT_PROP, errp); + if (!slot || !spapr_add_nvdimm(dev, slot, errp)) { goto out_unplug; } - spapr_add_nvdimm(dev, slot, &local_err); - } - - if (local_err) { - goto out_unplug; } return; out_unplug: pc_dimm_unplug(dimm, MACHINE(ms)); -out: - error_propagate(errp, local_err); } static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c index b3a489e9fe18..361ac8c28e33 100644 --- a/hw/ppc/spapr_nvdimm.c +++ b/hw/ppc/spapr_nvdimm.c @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, } -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) { SpaprDrc *drc; bool hotplugged = spapr_drc_hotplugged(dev); @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) g_assert(drc); if (!spapr_drc_attach(drc, dev, errp)) { - return; + return false; } if (hotplugged) { spapr_hotplug_req_add_by_index(drc); } + return true; } void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
As recommended in "qapi/error.h", add a bool return value to spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead of local_err in spapr_memory_plug(). Since object_property_get_uint() only returns 0 on failure, use that as well. Also call ERRP_GUARD() to be able to check the status of void function pc_dimm_plug() with *errp. This allows to get rid of the error propagation overhead. Signed-off-by: Greg Kurz <groug@kaod.org> --- include/hw/ppc/spapr_nvdimm.h | 2 +- hw/ppc/spapr.c | 35 +++++++++++++---------------------- hw/ppc/spapr_nvdimm.c | 5 +++-- 3 files changed, 17 insertions(+), 25 deletions(-)