Darren Reed wrote:

> 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.

The reason I wanted to do the import is that the branch is for a
rather large portion of the kernel, and I do not want to maintain
it outside -current longer than needed.

At the moment I believe it's in a state where it's stable and performs
good enough to be transparent to users. That's all. The real work
is ahead; so far it's just changing the interface and related code.

The issues you point out can be solved in the branch or in -current,
and they will be solved, but they are definitely not critical issues
that must be resolved before merging the branch, and as such, are
irrelevant for this discussion, IMHO...

> For example...?

falcon:sys {228} grep -r kauth_cred_getrefcnt * | cut -d':' -f1
coda/coda_namecache.c
coda/coda_namecache.c
coda/coda_vnops.c
compat/darwin/darwin_sysctl.c
compat/darwin/darwin_sysctl.c
kern/kern_auth.c
nfs/nfs_socket.c
nfs/nfs_subs.c
sys/kauth.h
falcon:sys {229}

CVS history should help.

> 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.

I haven't given enough thought to function naming yet; these are just
easy names for I did the crcopy -> kauth_cred_copy change. If you want
to look what each is doing, analzye why each caller needs it, perhaps
cleaning up some callers, and finding a better name - thanks! :)

> 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.

No. NetBSD pre-kauth had no need for a destroy routine because it
could declare local static struct ucred/pcred as it needed. NetBSD
post-kauth needs to get these from the kauth pool, and that is done
using kauth_cred_alloc() for now. Because this is, as you noted,
used locally for whatever the function is doing, before exit it should
free them back -- hence, the need for a destroy routine.

Think of it like kauth(9)'s version for malloc/free.

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

That's on the todo list.

> 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.

Small steps, Darren.

-e.

-- 
Elad Efrat