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

On 25/03/2014 4:39 a.m., Alex Rousskov wrote:
> 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.

Ouch. Sorry. I checked knotter but did not see anything, it seems to
have been down for a bit though.

No particular urgency. Just that I thought it was done, checked out okay
and was hoping to move on updating the parser work on top of the new buffer.

I will make a push to get the compile issues gone in the project branch.
If you want it reverted from trunk until review I can do that meanwhile.

Amos

>
>> 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 Wed Mar 26 2014 - 04:05:59 MDT