On Sun, 23 Nov 2014, Carlo Wood wrote:

Howdy, some thoughts on this awesome patch...

As mentioned before: I like the general approach of this as it offers the
application a fairly large degree of pipelining control and yet doesn't
overload libcurl with a new busload of options. This is probably what we
should've done earlier...

CURLMOPT_PIPELINE_POLICY_FUNCTION really need its own man page too that
describes how it is used.

> +#define CURL_SUPPORTS_PIPELINING 1
> +#define CURL_BLACKLISTED 2

I would like these defines to have a prefix that shows that they are used in
the curl_pipeline_policy struct only and I wouldn't mind using values that
show they are used within a bitmask. Like perhaps:

#define CURLPIPELINE_SUPPORTED (1<<0)
#define CURLPIPELINE_BLACKLISTED (1<<1)

> struct curl_pipeline_policy

1. I would like us to not use the same struct defintion internally as
externally because it makes it too easy for us to make a mistake in the future
and then break backwards compatiblity. Hence, I want us to populate the struct
before calling the callback and reading back the results aftward into a
different struct.

2. Also, passing in the pointer to actual the struct in use internally
unfortnately makes it a bit too easy for applications to keep writing to it
(after the callback returned control back to libcurl) and having that affect
libcurl later on.

3. I would like to add an 'age' field as its first struct member much in the
style we do for curl_version_info so that we can potentially add struct
members in the future and allowing applications to still do the right thing
with such updated structs.

4. Wouldn't it also make sense to pass in the Server: name to the callback, as
that's what's used for "site blacklisting" already. I.e. clients can/do use
the server name to make pipelining decisions.

> CURLMOPT_PIPELINING_SITE_BL

Your patch changes behavior of this option. Is that really necessary? First,
neither the old nor the new behavior is documented in the man page for
CURLMOPT_PIPELINING_SITE_BL. With this change, it'll change all existing
connections as well while it previously only affected future connections.

This switch in behavior will make it somewhat complicated for applications to
know how to handle this list if they are to support older libcurl versions
too, won't it?

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html