Re: [PATCH 2.6.23-mm1] Change the ida/idr_pre_get() return value to follow the kernel convention

[Pierre Peiffer <pierre.peiffer@bull.net>]

Andrew Morton wrote:
> On Tue, 30 Oct 2007 17:13:50 +0100
> Pierre Peiffer <pierre.peiffer@bull.net> wrote:
> 
>> ida_pre_get() and idr_pre_get() currently return 0 in case of error, and 1
>> in case of success, what is not the conventional way to handle error cases.
>>
>> This patch makes both of them return 0 in case of success, and an errcode
>> otherwise, and then it changes each caller to let them return the error
>> reported instead of ENOMEM. This avoids to the callers to make any assumption
>> about the cause of the error.
>>
> 
> If we're going to do this (and really we should), then we risk quietly
> breaking out-of-tree code and we risk breaking in-tree or
> soon-to-be-in-tree code which we didn't know about.

Indeed...

> So what we should do is to rename these functions when we change their
> interfaces.
> 
> Happily, this means that we then don't need to remove the old functions -
> we can keep them there for a while as we transition everything over to the
> new functions.  It also means that we can sneak the new functions into the
> 2.6.24 stream and then merge these changes via the relevant maintainers
> rather than needing a single atomic megapatch.
> 
> 
> Although I'd much prefer just to remove the pathetic things - idr_pre_get()
> is a truly awful interface.
> 
> It would be slightly better if it took an `id' argument and filled in the
> nodes at the appropriate position in the tree so that the caller is
> guaranteed that the subsequent idr_get_new() will succeed.  Because at
> present there is no guarantee that the nodes which you preallocated with
> idr_pre_get() are still available when you do your idr_get_new(): some
> other CPU/task might have come in and used them all.

Yes, that's what I've understand, indeed.

> 
> Storage classes which need to allcoate memory at insertion time are hard:
> radix_tree_preload() gets it right in terms of robustness, but it's an
> awful lot of fuss.
> 
> IDR gets it all wrong and compounds the problem by implementing internal
> locking.  It shouldn't have done that: storage code like this should use
> only caller-provided locking.

Ok, but for that, I prefer to let the IDR maintainers see what they can do,
because I'm not familiar at all with the IDR implementation and can not focus on
that now.

So do you think that just providing a new API (something like
idr_pre_allocate(), as you say above) would be better than nothing ?
Or we just leave the code as is for now ?

-- 
Pierre Peiffer
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/