Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection Filters v0.1 #9855

Closed
wants to merge 1 commit into from
Closed

Connection Filters v0.1 #9855

wants to merge 1 commit into from

Conversation

icing
Copy link
Contributor

@icing icing commented Nov 5, 2022

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
Copy link

lgtm-com bot commented Nov 5, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 7, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 7, 2022

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

new alerts:

  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Nov 8, 2022

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

new alerts:

  • 1 for FIXME comment

.gitignore Outdated Show resolved Hide resolved
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.
Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo 'bet' = 'be'

Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@icing icing Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

And this allows the stacking, as in:

```
Direct:
Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@icing icing Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somewhat related - would filters be 'pausable' ?

Copy link
Member

@bagder bagder Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

/* A connection filter instance, e.g. registered at a connection */
struct Curl_cfilter {
Copy link
Member

@xquery xquery Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xquery
Copy link
Member

xquery commented Nov 8, 2022

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
Copy link
Member

bagder commented Nov 9, 2022

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

struct Curl_easy *data);


CURLcode Curl_cfilter_create(struct Curl_cfilter **pcf,
Copy link
Member

@bagder bagder Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@icing icing Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

vszakats commented Nov 10, 2022

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

bagder
bagder approved these changes Nov 11, 2022
                    - 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.
@bagder bagder closed this in dafdb20 Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants