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

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:Hey all,
:
:I've ported my patch for the local kernel panic that I fixed in a patch 
:for FreeBSD over to DragonflyBSD as well. It's available at 
:http://sitetronics.com/lockfix.dfly.tar.gz. I'd appreciate feedback for 
:this .
:
:Please note that this is pretty much a literal copy of the patch I made 
:for FreeBSD. It hasn't been thoroughly reviewed by that crowd, so it's 
:probably not a bad idea for you guys to do that too .
:
:Let me know if anything needs to change.
:
:Kind regards,
:
: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')

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

    * diff -ur sys/kern/kern_lockf.c sys_lockfix/kern/kern_lockf.c

	+               if (ap->a_op == F_SETLK)
	+                       if (!chgposixlockcnt(pp, 1, lim_max(pp,
	+                           RLIMIT_POSIXLOCK)))
	+                               return (ENOLCK);
	+               else {
	+                       /*
	+                        * If the user isn't performing an F_SETLK operation,
	+                        * this structure will be freed, no matter the outcome
	+                        * of the operation.
	+                        */
	+                       chgposixlockcnt(pp, 1, 0);
	+               }

	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.

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

    * You have a lot of places where you do this:
	(diff -ur sys/kern/kern_lockf.c sys_lockfix/kern/kern_lockf.c)

	if (lock->lf_flags & F_POSIX)
		chgposixlockcnt(pp, -1, 0);
	free(lock, M_LOCKF);

      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.

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

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

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

					-Matt
					Matthew Dillon 
					<dillon@xxxxxxxxxxxxx>