Re: [PATCH] Convert the ConnStateData input buffer to SBuf

On Sun, 23 Mar 2014, Amos Jeffries wrote:

> If there are no objections I will commit this shortly.

I could not review the adjusted patch in time for your commit due to
travel. I tried to warn you about this on IRC, but it probably did not
work. I do not really understand the urgency with getting this change
committed, especially since significant changes were required to the
initial submission. For the future, perhaps we can adjust the
commit-after-objections rules to avoid such situations.

Alex.

> On 16/03/2014 1:08 a.m., Amos Jeffries wrote:
>> On 9/03/2014 7:34 p.m., Alex Rousskov wrote:
>>> On 03/08/2014 04:13 AM, Amos Jeffries wrote:
>>>> Prepare the way to efficiently parse client requests using SBuf based
>>>> parser-ng.
>>>>
>> <snip>
>>>
>>> Please redesign to avoid these problems. Please let me know if you need
>>> help with that -- I have a couple of ideas how this can be done, but I
>>> suspect yours are going to be better since you have spent a lot more
>>> time with this code.
>>>
>>
>> Good points.
>>
>> Attached patch extends the earlier one so IoCallback stores a
>> raw-pointer to the ConnStateData::In::buf.
>>
>> This is now specifically to the SBuf member object rather than its
>> MemBlob or char* backing stores. So only the short (blocking)
>> FD_READ_METHOD() call needs to provide any synchronous guarantees.
>> We particularly need a raw-pointer to the ConnStateData member to
>> prevent the same possible read/consume collisions causing problems when
>> it comes to merging the two separate SBuf later (by keeping only one SBuf).
>>
>>
>>>
>>>> - comm_read(clientConnection, in.addressToReadInto(), getAvailableBufferLength(), reader);
>>>> + comm_read(clientConnection, in.buf.rawSpace(in.buf.spaceSize()), in.buf.spaceSize()-1, reader);
>>>
>>> If this code survives, please allocate the buffer outside the comm_read
>>> call. There was already an ugly dependency on the right parameter
>>> evaluation order, and your changes make it look even worse.
>>>
>>
>> It is moved inside the deep read operations now, just before
>> FD_READ_METHOD is used. If I am understanding what you meant then that
>> should fix it properly.
>>
>> Amos
>>
>
Received on Mon Mar 24 2014 - 15:39:44 MDT