Re: [PATCH] libecap v1.0 support

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.

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).

For my edification can you explain a bit more please about why libecap
needs an entire EventLoop engine all of its own?
  What is so different about engines that you cant simply use scheduled
Calls and events from ecap?

Amos
Received on Thu Oct 31 2013 - 20:47:52 MDT