On Mon, Mar 20, 2006 at 11:20:59AM +0200, Elad Efrat wrote:
> Hello Darren,
> 
> > You have a kauth_cred_getrefcnt().  Why?
> 
> It's just a replacement for pc->p_refcnt. The idea was to first get a
> working tree and then work on cleaning up and removing things that are
> not needed. If you'd take a look at some of the commits to the branch
> you'd notice that this was done, mostly.

ok, then there's a bunch more work to do on cleanup, quite possibly
in fixing older NetBSD code that just has bad logic in it.

Since you are introducing a new interface it would be a good time
to get rid of as much cruft from cred_t and friends, if possible.

> > Why do you want to return the refcnt?  Why do you care if the value
> > is 4 or 5? 
> 
> I couldn't care less about it, but some code uses it.

For example...?

> > You have both kauth_cred_hold() and kauth_cred_dup().  Why ?
> 
> Same reason...
> 
> > i.e. rather than doing:
> > 
> > kauth_t *k;
> > 
> > k = something->kauth;
> > kauth_cred_hold(k)
> 
> ...and simply raising the reference count,
> 
> > do
> > 
> > kauth_t *k = kauth_cred_dup(something->kauth)
> 
> and allocate a new kauth_cred_t object from the memory pool.

Ok, so maybe I got confused about the names here.

To me, _clone should be _copy.
Its behaviour is more like memcpy in that regard.

Maybe you should call kauth_cred_copy something like kauth_cred_private,
because it seems like the intention is to give the call its own private
copy of the kauth_cred_t structure.

> > If kauth_cred_destroy() is never called directly, remove it from
> > the man page and update the description of _free()
> 
> But it *IS* called directly, see nfs/nfs_socket.c; maybe that'll
> change, for now it stays...

If I look at NetBSD pre-kauth, there is no equivalent to your
kauth_cred_destroy.  Given you have alloc/dup/copy/free, you
should not be exposing destroy as part of your API.  The way
you have used destroy in there suggests that you're not quite
sure what the reference count should be, is it 1 or is it 2 or
is it something else, just that whatever it is you don't care
about, the structure must go.  This kind of defeats the purpose
of using reference counts.

If you like, the presence of kauth_cred_destroy() is a bug in the API.

> > In the description of kauth_cred_copy, when you say clone, do you
> > mean you call kauth_cred_clone() ?  Otherwise, what is the purpose
> > of this function?  The description seems, to me, confused, with
> > the "reference count of 1" special case.
> 
> Yes. All the issues you are pointing out are *really* issues with the
> way Unix had done credentials. The kauth(9) interface routines you point
> out are simple drop-in replacements.

Well, maybe you should clean up NetBSD too, while you're at it :)

I understand that you started off to simply replace the old mechanism
with something new but since you are introducing something new, it is
worthwhile getting the kinks out and making it sensible rather than
continuing the perverse logic from before.

Darren