"Mindaugas R." <rmind@NetBSD.org> writes:

> Greg Troxel <gdt@ir.bbn.com> wrote:
>> 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.
>
> Actually, it does not matter since the memory is zeroed. But yes, I will set
> msg_type for the readability. However, moving such special cases out of the
> loop where appropriate makes it more readable for me.

It's fine with me; I just like it to be easy for a reviewer to be sure,
even at the expense of lines that could be omitted.

>> 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?)
>
> I am not sure what do you mean. Do you mean a correct counting of offsets?
> In such case - yes. Also, take a look at msginit() or seminit().

The code mallocs a large area that is the sum of sizes, and then uses
the sizes to grab sub-areas.  This does not guarantee that all of the
sub-areas will have appropriate alignment for their data structures.
For example, if the second area were char foo[17], the third area
wouldn't even be byte aligned.  I don't know what's in all the structs,
and they're probably all 8-byte aligned, but this seems in general
not safe.

> Re sparc64: No. Volunteer to test? :)

Sorry, I don't have one set up that I can test on right now - both of
mine that are running are in production with release branches.

>> 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.
>
> In case there are waiters, msgrealloc() might sleep in cv_wait(). At that
> moment, another msgrcv() call might happen which waits on cv_wait_sig().
> This check disallows such situation.

I'm willing to take your word for it; I'm a fan of more comments about
things that are hard to understand - but I haven't studied the new cv_*
regime at all, and arguably knowing that is a prereq for being a proper
reviewer.