[squid-dev] [PATCH] implement RFC3986

[Kinkie]

On Sun, Feb 21, 2016 at 4:51 PM, Alex Rousskov
<rousskov at measurement-factory.com> 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 think the design decision to use templated
> escape functions to accomodate std::string-using helpers was wrong, but
> I am not bold enough to stop you from committing it. Much worse code
> goes in despite my objections or without proper audit.

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.

>> is caused by the fact
>> that we are discussing about two topics at the same time.
>> One is: should Rfc3986 target both SBuf and std::string, if the
>> performance and complexity penalty in doing so is none or small? I
>> (and I believe Amos) think it's a good, or at least not a bad idea.
>
> Any "Should I do X if X is good?" question is pointless. The question is
> "Should I do X?". I have already outlined why I think doing X is harmful.
>
>
>> The other is: should we use std::string or SBuf or c-strings in
>> helpers? My opinion is that std::string would be preferable, yours is
>> that SBuf is preferable, if any change is performed over the current
>> state of using c-strings. I think change over current state is a good
>> idea, you - I believe - are averse enough to using std::string that
>> would prefer no change.
>
> 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. 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.
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.
Performance is not a big concern when it comes to helpers, but I argue
that this approach would require more code, be less efficient, and
increase coupling between squid and helpers than the template-based
approach.
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).

>> Can we try to decouple the two discussions and see if this helps us
>> reach a consensus?
>>
>> My point of view is:
>> - having a more generic API costs us nothing - the code is compact,
>> readable and efficient regardless the genericity, so we should merge
>> Rfc3986.
>
> A single template costs virtually nothing. I speculate that the design
> decision to treat std::string-using helpers as the primary driving
> factor for Squid APIs will cost a lot long-term.

Let's be clear: Squid API are in no way influenced by this. However
Squid has a need (a rfc3986/1738 codec library) which also helpers
have. Using a template leverages the opportunity offered by the very
similar APIs that std::string and SBuf have to increase applicability
of the library.

> This is very similar to assert(string_size < 64K). The cost of that
> single line was nothing when it was written. However, the cost of the
> design decision that Strings can self-limit their size is a growing
> collection of CVEs. IMO, you are opening a Pandora box. The cost of
> lifting the lid is indeed negligible, but that cost is not what I am
> worried about.

I still can't see the long-term harm, while I can see the short-term
harm in not following my idea.
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
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).

>> - there is no performance or readability benefit in using SBuf in
>> helpers due to the way client code is structured.
>
> I agree regarding readability. SBuf is about the same or a little worse
> than std::string.

I agree

> 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.
BTW: if there was a way to reasonably integrate std::string and
mempools, that would be worth investigating. I haven't found it.

>> Also, we would need
>> to stub a lot of functionality it relies on (e.g. mempools,
>> statistics, cachemgr).
>
> I am not sure that is true, but even if it is true, it should not be
> important for this discussion. The vast majority of such refactoring,
> would benefit Squid. For example, if our SBuf code depends on cache
> manager, then we made a mistake that should be fixed.

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

-- 
    Francesco