[patch] POSIX advisory mode lock panic fix by Dfly

[Devon H. O'Dell]

    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 at xxxxxxxxxxxxx>

--Devon