Re: [PATCH] Use SBuf for tunnel.cc I/O

On 05/13/2014 04:03 AM, Amos Jeffries wrote:
> On 13/05/2014 10:46 a.m., Alex Rousskov wrote:
>> On 05/11/2014 11:16 AM, Amos Jeffries wrote:
>>> Unlike the char* API the buffer can be appended to before write(2) is
>>> completed and the appended bytes will be handled as part of the original
>>> write request.

>> That approach scares me because the caller has to be extra careful not
>> to leave the being-updated-area of the buffer in inconsistent state
>> across async calls. However, I doubt we have such complex callers today,
>> and the approach also reduces the number of usually pointless write
>> callback calls, so I am willing to experiment.

> If you have a better approach I would like to hear it.

The simple alternative is to use the current approach of fixed-size
Write() sequences. It is difficult for me to say which approach is
better, but it is clear which one is simpler and which one is scary.
Again, I do not object to experimenting with the new scary approach.

> The raw-pointer usage which appears to be scaring you is the existing
> status quo anyway...

The above "scare" has nothing to do with raw pointers.

> * SBuf API passes a reference/raw-pointer to the smart pointer SBuf
> instance. All operations on the MemBlob backing store remain valid.
> => requirement that the caller do nothing to the SBuf instance other
> than allocate a new one while write is scheduled.

I do not think there is such a requirement. In fact, such a requirement
would defeat the purpose of allowing SBuf updates during the Comm
writing sequence. Also, such a requirement is nearly impossible to
enforce because the caller may be destroyed for reasons outside its
control, destroying the SBuf instance with it.

> ==> Note this is the same requirement as made on both other API
> already.

Not exactly. None of the existing Comm write APIs allow the caller to
change the size of to-be-written data after the Write call (the scary
experimental part I have agreed to above) and all of them allow the
caller to completely and safely disassociate itself from the writing
sequence via providing the free function for the buffer (the buf2
objection discussed below).

All APIs can be abused or misused in similar ways, but I am not focusing
on abuse/misuse right now. Both the "scary" experiment and the buf2 mess
exist outside the intentional abuse areas.

>> You forgot to mention the fact that the new Write API, unlike the
>> existing two, relies on the caller to keep the buffer area alive. That
>> requirement, IMO, is a show-stopper. I strongly object to that change.
>
> If by "buffer area" you mean MemBlob that is not a requirement.

I mean the object pointed to buf2 and everything that objects points to.
If the caller destroys the object buf2 points to (which may happen for
many reasons, some of which are beyond the caller control), Squid will
crash or worse. I do not think we should design new APIs like that.

The caller can also change the object that buf2 points to (to append
more data for writing). Such changes are the "scary" part I agreed to
above. They scare me, but I do not object to them.

> One either sends an SBuf which is a member of the Job/*StateData object
> itself or an SBuf member of the Params object which is delivered to the
> callback. Both methods ensure its alive until at least the callback is
> scheduled.

If Comm was storing an SBuf object, the show-stopping problem would not
exist. With the buf2 design, Comm stores a pointer to an SBuf object
instead. I do not think that should be allowed. We worked hard on
SBuf/MemBlob API to prevent such storage.

>> Please avoid these and similar changes if they are unrelated to your
>> patch scope.

> With the removal of client.len/server.len byte accounting the
> delayId.bytesIn() is now used directly for delay pool byte accounting.
> Which makes the delayId wrappers of all types needless.

Please explain that in the proposed commit message. It is not a good
idea to force others to guess whether these seemingly unrelated changes
are accidental or intentional.

>>
>>> - debugs(26, DBG_DATA, "Tunnel server PUSH Payload: \n" << Raw("", tunnelState->server.buf, tunnelState->server.len) << "\n----------");
>>> + debugs(26, DBG_DATA, "Tunnel server PUSH Payload: \n" << tunnelState->server.buf << "\n----------");
>>
>> Please do not remove Raw() protection for dumping raw data.

> SBuf provides generic operator<<(std::ostream&) for debugs.

I know. That generic operator is not suitable for dumping raw data. Use
Raw() for that. I believe Raw() can be used with SBufs as-is, but you
can also add an SBuf-friendly version of Raw() if you prefer to add that
to the patch scope.

> If Raw() protection is required on the buffer it needs to be provided by
> that.

No, it should not be provided by that generic << operator. That operator
is for writing "meaningful" strings. Raw() is for debugging/dumping
raw/opaque data. We have opened this Pandora box when we agreed that the
SBuf class will be used for both meaningful strings and raw data
(instead of having dedicated classes, one for each).

HTH,

Alex.
Received on Tue May 13 2014 - 15:27:46 MDT