[squid-dev] [PATCH] implement RFC3986

[Alex Rousskov]

On 02/22/2016 02:53 PM, Kinkie wrote:
> On Sun, Feb 21, 2016 at 4:51 PM, Alex Rousskov wrote:
>> On 02/20/2016 11:27 AM, Kinkie wrote:
>>> Sorry to bring this topic up again, but honestly I don't understand
>>> your position.

>>> I believe that the deadlock we currently are in

>> There is no deadlock.
>> I am not bold enough to stop you from committing [your changes].

> Unfortunately there is: I am prepared to abandon this effort unless we
> reach consensus - at least non-opposition. It'd be a pity, as it's an
> useful feature.

This is not a deadlock. You have many possible ways forward, including:

1. Commit your changes despite my opinion.

2. Suspend your changes and work on the infrastructure to enable similar
functionality in the future but without the alleged drawbacks of the
current approach.

3. Work on changing my opinion.

I am not sure what you want from _me_ though. I have detailed my opinion
more than once. I am not blocking you. If, in addition to that, you also
want me to feel *happy* about the direction of your changes, then you
need to master remote mind control or come up with some other way to
change my opinion.


>> I think helpers should use whatever works best for them, given the
>> available Squid APIs and other factors. The question is not about what
>> helpers should use but whether Squid code should bend over backwards
>> (e.g., providing a templated function to escape strings and adjust SBuf
>> to work with that function) to accommodate a helper. The answer, IMO, is
>> "no". It is much better to
> 
>> * provide SBuf to helpers that want top performance
>>
>> and
>>
>> * provide a trivial std::string-escaping function (that uses
>> SBuf-escaping function internally) to helpers that want to use C-strings
>> or std::strings.


> Ok, I believe here is the crux of the issue.

I do not think so. The crux of the issue is that you are defending a
medium-size template function as a necessary and small evil, while I am
attacking the principle or direction of accommodating helper needs by
making Squid code worse.


> rfc3986 is a library; we
> could split it into two versions, one to be used by helpers and one to
> be used by squid. The two versions would be remarkably similar and
> have a lot of code duplications, or as you propose be one an adapter
> of the other.

Those are not the only two options, but yes, those two options do exist.


> An adapter exposing a std::string API and using SBuf internally would
> be quite inefficient as it'd need to copy data back and forth.

Yes.


> Performance is not a big concern when it comes to helpers, 

There are probably Perl, Python, Javascript, etc. string-escaping
libraries available for helpers that are not especially concerned about
performance.


> but I argue that this approach would require more code, 

That depends. I do not think the wrapper/adapter approach requires more
code. It probably requires less code long-term. The wrapper itself is a
couple of simple lines. The SBuf-focused implementation may be even
shorter than the proposed template.

It is also a pretty vague statement. More code in headers where every
line ought to count as three (if we are counting lines)? No. More
duplicated code? No.

[ And if a helper uses either naked SBuf or some 3rd party std::string
library for escaping then there is no "more code" at all! ]


> be less efficient, and

Yes, but you said it is not a big concern during this discussion.


> increase coupling between squid and helpers than the template-based
> approach.

Yes. Some might consider that to be a good thing for areas where helpers
and Squid have to do similar things.


> In order to clarify, when I talk about couplign I mean link-time
> coupling. This can be directly understood from the list of objects
> needed for testSBuf: between headers, objects and stubs that's about
> 30 files (on top of the testSBuf files themselves).

If testSBuf has 30 dependencies, then I doubt testSBuf is a good example
of how things should be done. Overall, if we want helpers to use Squid
code, SBuf can be a part of the convenience library that can be linked
with helpers without 30 "extra" or "stub" files. The number of files in
that convenience library is pretty much irrelevant.


> I still can't see the long-term harm

Yes, that is why we disagree. You think the decision making stops with
your escape function. I think it starts there. Once Squid begins
accommodating helpers by doing crazy things like writing templated
functions to escape a string, it will be difficult to stop. There will
be more and more cases where a helper needs something from Squid, but
refuses to use Squid APIs to get it. And we will spend more and more
time on workarounds and Squid core changes just to make helpers happy.


> To the cost of repeating myself, we want a SBuf API for squid where
> performance and COW matter a lot. I wish a std::string API for helpers

Yes, that is understood. I am not sure you would be happy when your wish
is granted though. It is not clear to me that helpers that we write in
C++ and that do things similar to what Squid does, should use something
other than SBuf that Squid should be using everywhere.


> to avoid having to link 30+ squid files and stubs for each helper
> (object size is not a concern, it's more about maintaining this long
> list of dependencies).

I am oversimplifying a little, but there should be only one dependency:
"libsquidstring.la" or "libsquidbase.la" or some other "core" library
like that. The number of files in that convenience library is not the
issue IMO because all of them are used by Squid anyway.

The correct first step, IMO, would be to fix the "30 stubs" problem
instead of working around that problem by creating templated escape
functions. Yes, the former is more difficult. Squid code is full of bugs
stemming from basic development principles violation because, in part,
it is "easier" to write code that way.


>> If you are right about performance and safety across all helpers, then
>> there is no benefit in using SBuf in Squid.
> 
> Here I disagree. SBuf is relevant for squid. It is not for helpers.

Why is a faster string designed for parsing/packing not relevant to
helpers as far as performance and safety are concerned?


> It doesn't depend from cachemgr to work, but it links to it. This can
> probably be refactored to decrease decoupling.

s/can probably be/should be/

And please note that we cannot use the usual "it was broken before me"
excuse because we created SBuf from scratch! It did not have to depend
on anything unrelated to SBuf, such as a cache manager, but it was
probably "easier" to create such dependencies than to avoid them.

Alex.