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

On 06/01/2014 02:37 AM, Amos Jeffries wrote:
> Lets take a step aside and cover why the buf2 is a pointer instead of an
> separate SBuf.

I do not think those reasons are important enough in this context. It is
like suggesting "let's examine the benefits of using uninitialized
variables and creating dangling pointers" -- whatever the benefits are,
they are insufficient to knowingly add such code to Squid.

Giving Comm a raw pointer to an internal job member is very similar to
"using uninitialized variables and creating dangling pointers". No
amount of resulting optimizations can justify adding that kind of API IMO.

> * simple solution and ony way to guarantee that data-copy is not
> performed is to have comm_read operate on a pointer to sb1.

AFAICT, among correct solutions, there is no simple solution that avoids
data copy. We can either remain "simple" and copy OR add "complexity" to
avoid copying.

> comm_write() is a slightly different story. It *is* possible to take sb2
> as a copy then the write handler sb1.consume(len) for the written bytes.
>
> However, since read has the "SBuf *buf2" as a pointer

... so we are back to the discussion above: Since I do not think having
buf2 pointer should be allowed, any conclusion based on having it there
becomes irrelevant.

> If you can provide a way to avoid all the above problems while
> performing comm_read() on a non-pointer IoCallback "SBuf buf2;" I am
> very interested.

I do not think me providing a solution to all problems should be a
precondition to removing unaudited code that introduces a raw pointer
shared between Comm and jobs. If I understand the intent behind the
audit procedures correctly, it suggests the opposite: removal of the
code until a better solution is agreed upon.

There are no simple solutions here and the complex ones require serious
discussions and significant investment of time. IMO, there are better
ways to spend those resources right now. In other words, there are more
important problems to fix. Removing a data copy while crossing Comm
boundary can wait, especially if nobody wants to take a lead on that
project. Meanwhile, we can add a safe SBuf-friendly API and continue to
copy.

Alex.
Received on Mon Jun 02 2014 - 18:39:49 MDT