Re: [patch] POSIX advisory mode lock panic fix by Dfly
> 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@backplane.com>
>
--Devon
討論串 (同標題文章)
完整討論串 (本文為第 1 之 17 篇)：