[Openais] Re: recent segfault

Mark Haverkamp markh at osdl.org
Thu Feb 3 14:51:52 PST 2005


On Thu, 2005-02-03 at 15:12 -0700, Steven Dake wrote:
> Mark
> 
> Cool I'll start testing with it.
> 
> I have noticed there is definately still some bugs; I think related to
> recovery in totemsrp.  I've got a handle on these cases, but I'm not
> quite sure how to fix them yet.  I did notice a possible bug in your
> patch when I looked at it earlier.
> 
> In the previous patch you sent I noticed a problem and fixed it.  I
> think this code has the same problem.  Here is the code:
> 
>     for  (i = 0; i < msg_count; i++) {
>         /*
>          * If the first packed message is a continuation
>          * of a previous message, but the assembly buffer
>          * is empty, then we need to discard it since we can't
>          * assemble a complete message.
>          */
>         if (mcast->continuation && (assembly->index == 0)) {
>             assembly->index += msg_lens[i];
>             mcast->continuation = 0;
>             continue;
>         }
> 
>         assembly->index += msg_lens[i];
>         app_deliver_fn (source_addr, &iov_delv, 1,
>             endian_conversion_required);
>         iov_delv.iov_base = &assembly->data[assembly->index];
>         iov_delv.iov_len = msg_lens[i + 1];
>     }
> 
> in the case of continuation being set, assembly->index is increased. 
> Shouldn't iov_delv.iov_base and iov_delv.iov_len should also be moved to
> the next message?
> 

> Something like:
> 
>         if (mcast->continuation && (assembly->index == 0)) {
>             assembly->index += msg_lens[0];
>             mcast->continuation = 0;
> 	iov_delv.iov_base = &assembly->data[msg_lens[0]];
> 	iov_delv.iov_len = msg_lens[1];
>             continue;
>         }

Yes, I missed that.

> 
> Also it isn't valid to change mcast->continuation.  This value must be
> cached if you intend to change it.  The reason is that it may be
> delivered to one processor, but then another processor requests it be
> retransmitted.  If that happened, the retransmit list would contain the
> modified header which would .

That makes sense, but I saw that you did a swab16 on the count values in
the endian_coversion_required if statement which changed them, so I
thought that it would be OK.

> 
> All data in the delivered message must be read only for this reason.
> 

-- 
Mark Haverkamp <markh at osdl.org>




More information about the Openais mailing list