Re: [patch] POSIX advisory mode lock panic fix by Dfly

["Devon H. O'Dell" <dodell@xxxxxxxxxxxxxxx>]

>     Hi Devon!  Yes, I was following that conversation on the freebsd
>     lists.  Here is my review:  It looks like you are making progress
>     but the chgposixlockcnt() abstraction needs to be cleaned up a bit.
>
>     * Did you mean to add a 'k' here instead of a second 'l' ?
>
> 	diff -ur bin/sh/miscbltin.c bin_lockfix/sh/miscbltin.c
> 	-       while ((optc = nextopt("HSatfdsmcnuvlb")) != '\0')
> 	+       while ((optc = nextopt("HSatfdsmcnuvlbl")) != '\0')

Indeed. Damn typos :).

>     * There may be some bootstrapping issues in login.conf if the
>       new posixlocks entry is added prior to the system being updated.

I'll look into this.

> [snip]
> 	always use braces when enclosing a multi-line statement in a 
> 	conditional or loop, and it's also a good idea to use braces in
> 	the if() portion if the else portion uses braces e.g. 
> 	if (ap->a_op == F_SETLK) { ...  Even though the compiler
> 	understands it, code needs to be human readable too.

Okay :).

>     * Also, refering to the same code fragment above, the comment is a bit
>       confusing.  Perhaps it should read 'since this structure will soon
>       be freed unconditionally do not bother checking the resource limit
>       for this case'.

Per your other emails, I'm implementing a patch that changes this behavior.

>     * You have a lot of places where you do this:
> [snip] 
>       Why not simply create an integrated 'lf_freelock()' function which
>       does the lock count correction and the free() call instead of 
>       duplicating the code in a dozen times?  It is also good programming
>       practice to funnel counting operations into a single procedure if
>       possible (or two, one for allocation/increment, one for deallocation/
>       decrement) instead of separating the allocation and increment 
>       components and deallocation and decrement components, which could lead
>       to reference counting bugs.

See above :).

>     * Also in regards to the 'chgposixlockcnt' function, it appears (but I
>       haven't checked all the cases) that the first argument is always
>       ((struct proc *)lock1->lf_id).  Instead of putting 'struct proc *pp'
>       declarations all over the place would it make more sense to simply pass
>       the struct lockf * pointer as the first argument instead of a
>       struct proc ?  (This would also be moot if the lock counting were
>       integrated into the lockf structure allocation and deallocation
>       functions).

Well, the number of locks needs to be kept track of on a per-process 
level as well for possible setuid() transfers. I think that passing a 
struct lockf * is a good idea; but it's not moot unless the process 
count is upgraded in the lf_alloc() instead of in chgposixlockcnt() (but 
I don't think that's very clean, is it?)

>     * Also in regards to the 'chgposixlockcnt' function, I recommend
>       splitting it into two functions (as well as integrating it into the
>       lock structure allocation and freeing code): one to add refs,
>       and another one to subtract them, rather then conditionalizing
>       it within a single function (which eats cpu cycles unnecessarily).

Okay. At first, I didn't understand your point here; but I see what you 
mean now. I'll take this into account.

>     * The new return code '2' for lf_split() is not documented.
>       (diff -ur sys/kern/kern_lockf.c sys_lockfix/kern/kern_lockf.c)

I was simply returning the number of new locks when the function exits. 
Joerg's implementation is better.

> 					-Matt
> 					Matthew Dillon 
> 					<dillon@xxxxxxxxxxxxx>

--Devon