Subject: Re: Dynamic limits for SysV semaphores and message queues
To: Mindaugas R. <rmind@NetBSD.org>
From: Greg Troxel <gdt@ir.bbn.com>
List: tech-kern
Date: 10/07/2007 07:00:13
You have hoisted the special case out of the loops, and I think in one
case this introduced a bug: msghdrs[msginfo.msgtql-1].msg_type doesn't
get set to 0.  I'd prefer to leave the loops whole and use the if in
these cases, as it's clearer to verify correctness.  I think your change
to make the top element the special case one helps understandability.

EPERM for not the new memory being smaller seems like the wrong code,
because the user might well have permission, but the kernel is enforcing
an invariant even on root.  If I follow, this is when a user asks for
the # of semaphores to be smaller than one which is currently in use.
So it feels more like EBUSY, EINVAL, EEXIST, or EOVERFLOW.  While I
realize this is a tough call, I tend to want to make it easier for
someone who is losing to grep the kernel to find out which test failed.

Does the new_msgpool = ptr + size idiom adequately ensure alignment?  If
it does because of the structure contents, a comment explaining why
would be nice to perhaps avoid future problems.  Or a KASSERT about
size/alignment.  (Did you try this on sparc64?)

Again on realloc "new_msghdrs[i].msg_type = 0;" does not get executed
for the highest new_msghdrs[i].

I don't see how the if(msg_realloc_state) (at 455/659) interacts with
reallocation, because it looks like msgmutex takes care of this.

I didn't read the rest, or even the above very carefully, but I'm glad
to see dyamic sizing implemented - I had to turn down the number of
mutex_locks for mod_python to get it to run.  My comments are really
very minor.