Connection Filters v0.1 #9855

icing · 2022-11-05T12:36:43Z

Connection filters (cfilter) addition to curl:

    - general construct/destroy in connectdata
    - default implementations of callback functions
    - connect: cfilters for connect and accept
    - socks: cfilter for socks proxying
    - http_proxy: cfilter for http proxy tunneling
    - vtls: cfilters for primary and proxy ssl
    - change in general handling of data/conn
      - Curl_cfilter_setup() sets up filter chain based on data settings, if none are installed by the protocol handler setup
      - Curl_cfilter_connect() boot straps filters into `connected` status, used by handlers and multi to reach further stages
      - Curl_cfilter_is_connected() to check if a conn is connected, e.g. all filters have done their work
      - Curl_cfilter_get_select_socks() gets the sockets and READ/WRITE indicators for multi select to work
      - Curl_cfilter_data_pending() asks filters if the have incoming data pending for recv
      - Curl_cfilter_recv()/Curl_cfilter_send are the general callbacks installed in conn->recv/conn->send for io handling
      - Curl_cfilter_attach_data()/Curl_cfilter_detach_data() inform filters and addition/removal of a `data` from their connection
      - adding vtl functions to prevent use of Curl_ssl globals directly in other parts of the code.

Fixing bugs found by building with cares:

- report successul connects only once
- use accept cfilter also in ftp port commands
- have accept cfilter take a new socket after accept() happened
- fix FILE handler to not leak memory when handler->connect_it() is called twice

lgtm-com · 2022-11-05T12:54:13Z

This pull request introduces 1 alert when merging 653e30a into 6b6667c - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-05T17:29:54Z

This pull request introduces 1 alert when merging 4ce9964 into 6b6667c - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-05T20:47:22Z

This pull request introduces 1 alert when merging a802588 into 6b6667c - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-06T13:30:47Z

This pull request introduces 1 alert when merging 462afe8 into 592107f - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-06T14:43:55Z

This pull request introduces 1 alert when merging 355a548 into 592107f - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-06T20:08:37Z

This pull request introduces 1 alert when merging 02fd43d into 592107f - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-07T11:45:38Z

This pull request introduces 1 alert when merging a19c3e5 into af5a22a - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-07T17:03:48Z

This pull request introduces 1 alert when merging dc08fc4 into ec4eec2 - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2022-11-08T11:44:07Z

This pull request introduces 1 alert when merging 8931045 into 43232b5 - view on LGTM.com

new alerts:

1 for FIXME comment

.gitignore

xquery · 2022-11-08T15:05:18Z

docs/CONNECTION-FILTERS.md

+A "connection filter" is a piece of code that is responsible for handling a range of operations
+in regard to curl's connections: reading, writing, waiting on external events, connecting and closing down - to name the most important ones.
+
+The most important feat of connection filters is that they can bet stacked on top of each other (or "chained" if you prefer that metaphor). In the very common scenario that you want to retrieve a `https:` url with curl, you need 2 basic things to send the request and get the response: a TCP connection, represented by a `socket` and a SSL instance en- and decrypt over that socket. You write your request to the SSL instance, which encrypts and writes that data to the socket, which then sends the bytes over the network.


typo 'bet' = 'be'

Generally I have an uneasy feeling about such a broad design concept being introduced ... for example saying 'stacked' or 'chained' does not provide enough concrete detail (for me) ... it is hinted at eg. it is called out the difference between transforming and non transforming pipelines (a set of filters implies a pipeline right ?) .... I am not looking for academic rigor but even a scant mention of Jackson Structured Programming or 'anything' might help constrain the scope of what is being attempted here.

thanks for noticing the typo.

As to the "uneasy" comment, I am not sure what you are looking for. Another concept than the one implemented or a better description of it?

no not another concept, just a more concrete description of the concept you are presenting ... pipelines of filters can be very attractive from a code organisation/syntax/composability pov ... just trying to grasp the operational impact ... and more importantly constraint on scope (esp on this initial commit).

docs/CONNECTION-FILTERS.md

xquery · 2022-11-08T15:57:33Z

docs/CONNECTION-FILTERS.md

+And this allows the stacking, as in:
+
+```
+Direct:


question (mostly to aid my understanding) - will these filter pipelines be static or dynamically composable ?

Right now, there is only the possibility to add filters. This is needed for STARTTLS. This is dynamic in the sense that, for example IMAP sets up its plain connection, connects() and starts talking. If it decides to do STARTTLS, it adds an SSL filter and calls connect() again until done. Then it can continue the conversation.

somewhat related - would filters be 'pausable' ?

Filters as implemented here don't introduce any new features. Things in curl remain "pausable" after this PR to the same extent they are today: a little.

xquery · 2022-11-08T16:25:08Z

lib/cfilters.h

+};
+
+/* A connection filter instance, e.g. registered at a connection */
+struct Curl_cfilter {


will we ever surface up generic filter concepts ex. timeout or debug callbacks ?

xquery · 2022-11-08T16:59:40Z

Generally I like filters as dynamic call chain and LGTM - if we ever bumped from C89 I might be inclined to be a bit more functional in approach (have not looked since gcc/clang nested functions/blocks where all this stands these days) but thats just waking dream I have from time to time. nice work!

Unasked for advice - go slow and constrain scope ;)

bagder · 2022-11-09T13:45:50Z

Windows build failures:

In file included from C:/projects/curl/lib/curl_setup.h:705,
                 from C:/projects/curl/lib/cfilters.c:25:
C:/projects/curl/lib/cfilters.c: In function 'Curl_cfilter_setup':
C:/projects/curl/lib/cfilters.c:291:12: error: 'index' undeclared (first use in this function)
  291 |            index, conn->bits.tunnel_proxy, conn->bits.httpproxy,
      |            ^~~~~
C:/projects/curl/lib/curl_setup_once.h:279:19: note: in definition of macro 'DEBUGF'
  279 | #define DEBUGF(x) x
      |                   ^
C:/projects/curl/lib/cfilters.c:291:12: note: each undeclared identifier is reported only once for each function it appears in
  291 |            index, conn->bits.tunnel_proxy, conn->bits.httpproxy,
      |            ^~~~~
C:/projects/curl/lib/curl_setup_once.h:279:19: note: in definition of macro 'DEBUGF'
  279 | #define DEBUGF(x) x
      |                   ^
make[2]: *** [lib/CMakeFiles/libcurl.dir/build.make:182: lib/CMakeFiles/libcurl.dir/cfilters.c.obj] Error 1

bagder · 2022-11-10T09:41:03Z

lib/cfilters.h

+                                 struct Curl_easy *data);
+
+
+CURLcode Curl_cfilter_create(struct Curl_cfilter **pcf,


Is there a pattern to when you use _cf_ vs _cfilter_ in symbol names? Would it not make sense to stick to _cf_ for consistency?

I tried to differentiate between calls on data, conn, index and call on a cf instance.

Maybe we move the Curl_cfilter_xxx() into url.c and rename to Curl_conn_xxx()?

vszakats · 2022-11-10T16:08:25Z

I'd throw in the idea to use the name cfilt instead of cfilter (e.g. Curl_cfilt_setup()). Easier to type, shorter, still easy to grep, and IMO still unambigiously meaning "filter". Another alternative: Not shorter, still easier to type and also clarifying c, could be connfilt (e.g. Curl_connfilt_setup()). [ My 2cents, and feel free to disregard. ]

- general construct/destroy in connectdata - default implementations of callback functions - connect: cfilters for connect and accept - socks: cfilter for socks proxying - http_proxy: cfilter for http proxy tunneling - vtls: cfilters for primary and proxy ssl - change in general handling of data/conn - Curl_cfilter_setup() sets up filter chain based on data settings, if none are installed by the protocol handler setup - Curl_cfilter_connect() boot straps filters into `connected` status, used by handlers and multi to reach further stages - Curl_cfilter_is_connected() to check if a conn is connected, e.g. all filters have done their work - Curl_cfilter_get_select_socks() gets the sockets and READ/WRITE indicators for multi select to work - Curl_cfilter_data_pending() asks filters if the have incoming data pending for recv - Curl_cfilter_recv()/Curl_cfilter_send are the general callbacks installed in conn->recv/conn->send for io handling - Curl_cfilter_attach_data()/Curl_cfilter_detach_data() inform filters and addition/removal of a `data` from their connection - adding vtl functions to prevent use of Curl_ssl globals directly in other parts of the code.

icing force-pushed the conn-filters branch from a802588 to 462afe8 Compare November 6, 2022 13:19

icing force-pushed the conn-filters branch from ddb5698 to a19c3e5 Compare November 7, 2022 11:33

bagder added the tidy-up label Nov 8, 2022

icing force-pushed the conn-filters branch from 53544a4 to d62a42b Compare November 8, 2022 11:26

icing force-pushed the conn-filters branch from 8931045 to 1851e4f Compare November 8, 2022 14:56