[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()
From: |
Greg Kurz |
Subject: |
Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug() |
Date: |
Thu, 17 Sep 2020 12:04:37 +0200 |
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...
>
- Re: [PATCH 13/15] spapr: Add a return value to spapr_check_pagesize(), (continued)
- [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Greg Kurz, 2020/09/14
- Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), David Gibson, 2020/09/16
- Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Markus Armbruster, 2020/09/17
- Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(),
Greg Kurz <=
- Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Markus Armbruster, 2020/09/17
- Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Daniel P . Berrangé, 2020/09/17
- Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Greg Kurz, 2020/09/17
- Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug(), Markus Armbruster, 2020/09/17
[PATCH 15/15] spapr: Simplify error handling in spapr_memory_unplug_request(), Greg Kurz, 2020/09/14
[PATCH 11/15] spapr: Simplify error handling in spapr_cpu_core_realize(), Greg Kurz, 2020/09/14
Re: [PATCH 00/15] spapr: Error handling fixes and cleanups (round 2), David Gibson, 2020/09/16