From: Tilman Schmidt <[email protected]>
Date: Sun, 31 May 2009 01:32:04 +0200 (CEST)
> I would like to propose the following three patches to the Kernel CAPI
> subsystem for inclusion in kernel release 2.6.31.
> The first one was proposed four weeks ago and so far hasn't met any
> protests.
> The second one is a rather trivial bugfix and documentation improvement.
> The third one just adds a few new insights about the workings of the
> CAPI subsystem to the Documentation file, INTERFACE.CAPI.
I'm not applying this patch series.
Especially I dislike the second patch.
First problem in the second patch. You're doing two things
at once. You're adding function documentation and also adding
a NULL pointer check.
Second problem, the NULL pointer check is gratuitous. Document
that the 'm' member has to be non-NULL and leave the check out.
Thanks.
On 01.06.2009 12:04, David Miller wrote:
> First problem in the second patch. You're doing two things
> at once. You're adding function documentation and also adding
> a NULL pointer check.
No problem. I can split the patch in two if you prefer it that way.
> Second problem, the NULL pointer check is gratuitous. Document
> that the 'm' member has to be non-NULL and leave the check out.
That would be a bad solution for two reasons:
First, the 'm' member is private to capiutil.{c,h}. Callers are
not supposed to access it. Therefore it shouldn't be referred to
in the interface documentation. At best, such a mention would
leave users of the function confused how to assure that condition.
At worst, it might mislead them into meddling directly with the
member, thereby producing incorrect code.
And second, the main use of capi_cmsg2str() is for error reporting
and debugging output. Oopsing in an error handler is particularly
troublesome. At the same time, the risk of the 'm' member being
unexpectedly NULL is particularly high when something has gone
wrong already. So a safety check is advisable in this case.
Thanks,
Tilman
PS: Any objections against the other two patches?
--
Tilman Schmidt E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Unge?ffnet mindestens haltbar bis: (siehe R?ckseite)
From: Tilman Schmidt <[email protected]>
Date: Sun, 07 Jun 2009 00:38:26 +0200
> On 01.06.2009 12:04, David Miller wrote:
>> First problem in the second patch. You're doing two things
>> at once. You're adding function documentation and also adding
>> a NULL pointer check.
>
> No problem. I can split the patch in two if you prefer it that way.
Thanks.
>> Second problem, the NULL pointer check is gratuitous. Document
>> that the 'm' member has to be non-NULL and leave the check out.
>
> That would be a bad solution for two reasons:
> First, the 'm' member is private to capiutil.{c,h}. Callers are
> not supposed to access it. Therefore it shouldn't be referred to
> in the interface documentation. At best, such a mention would
> leave users of the function confused how to assure that condition.
> At worst, it might mislead them into meddling directly with the
> member, thereby producing incorrect code.
> And second, the main use of capi_cmsg2str() is for error reporting
> and debugging output. Oopsing in an error handler is particularly
> troublesome. At the same time, the risk of the 'm' member being
> unexpectedly NULL is particularly high when something has gone
> wrong already. So a safety check is advisable in this case.
Fair enough.
> PS: Any objections against the other two patches?
I don't remember it's been so many days since I looked at
this series :-(