[patch] POSIX advisory mode lock panic fix by Dfly

Devon H. O'Dell dodell at sitetronics.com
Tue Apr 20 12:30:46 PDT 2004

    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.

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

					Matthew Dillon 
					<dillon at xxxxxxxxxxxxx>


More information about the Submit mailing list