Re: [PATCH] libecap v1.0 support

On 10/31/2013 02:47 PM, Amos Jeffries wrote:
> On 31/10/2013 1:16 p.m., Alex Rousskov wrote:
>> Hello,
>>
>> The attached patch upgrades Squid to libecap v1.0, allowing
>> asynchronous (e.g., threaded) eCAP adapters and eCAP version checks.
>>
>> After these changes, Squid can support eCAP adapters built with libecap
>> v1.0, but stops supporting adapters built with earlier libecap versions
>> (due to libecap API changes). The new libecap version allows Squid to
>> better check the version of the eCAP adapter being loaded as well as the
>> version of the eCAP library being used. This should help with migration
>> to libecap v1.0.
>>
>>
>> Thank you,
>>
>> Alex.
>
> In src/adaptation/ecap/Host.cc:
> * since both parameters passed to SupportedVersion() start out as char*.
> Please consider using SBuf instead of std::string and char* manipulations.

> - otherwise please at least use std::string() constructor to make the
> conversion explicit. It looks quite strange to have auto-conversion from
> char* to std::string& on the second parameter, but strangely not on the
> first parameter.

Char* type for the first argument is necessary because that argument may
be NULL (rather than empty) in legacy adapter cases. Both arguments are
using char* type now (with an explicit conversion to SBuf) to address
your "consistency" concerns.

> Other than the above the code appears consistent. The above can be done
> without needing abother round of review IMO.
> So I'm giving this a +0 (neutral about it going in as-is, the SBuf usage
> will make it +1).

Committed to trunk (r13155) after converting std::string to SBuf as
requested above and fixing "make check" dependencies.

Thank you,

Alex.
Received on Fri Nov 29 2013 - 19:55:08 MST