On 1/10/21 2:32 PM, Daniel Stenberg wrote:
On Sun, 10 Jan 2021, Patrick Monnerat via curl-library wrote:
Considering the large number of references, I would suggest starting
to fix this "bottom up" by smaller commits rather than having a big
patch, even if it increases the reference count at first while the
work is not complete.
Right. That's my thinking as well and a reason for not just removing
the 'conn->data' pointer at once. I want the removal to be the goal,
but the journey of getting to that point is allowed to take time.
However, it is also one of these architectual changes that once you
start to change two places you'll notice that you also need to update
a third place, which reveals the forth etc and all of a sudden the
change is massive and interconnected.
That's why I'm in favour of a bottom up approach. Example:
void abcd(struct connectdata *conn...)
{
struct Curl_easy *data = conn->data;
...
}
...
abcd(conn);
Could be turned at first as:
void abcd(struct Curl_easy *data)
{
...
}
...
abcd(conn->data);
Although this reintroduces a new conn->data reference, it can more
easily be removed at some following step. This will reduce the snowball
effect you mention above.
For things like handlers and TLS procedures (i.e.: where there are
several procedures that must have the same prototype signature), as
first (multiple) steps we can rewrite the procedures to turn:
void abcd(struct connectdata *conn)
{
...
}
Into:
static void real_abcd(struct Curl_easy * data)
{
...
}
void abcd(struct connectdata *conn)
{
real_abcd(conn->data);
}
And an additional step will change the calling sequences and rename the
real_*() into *() without having to touch the procedures' code again.
I started out by changing the protocol handler function pointers to
accept 'Curl_easy *' instead of 'connectdata *'. It took me down a
rabit hole for a while but I think I can breathe again now:
https://github.com/curl/curl/pull/6425
I'm currently preparing a PR against branch bagder/data-for-conn for
x509asn1 and gskit. Not yet complete.
I also noticed there are a lot of references to conn->data for
logging purposes only (infof, failf, debug) in connection-oriented
procedures: maybe we should discuss an alternate strategy for logging
from those procedures.
I don't think we have a lot of wiggle room to do that. Everything we
do in libcurl is oriented around a transfer and properties like
callbacks and VERBOSE are set in the transfer object. We can't output
any logs without also knowing the transfer!
As the current logging is implemented, I agree. That's why I suggested
alternatives.
-------------------------------------------------------------------
Unsubscribe: https://cool.haxx.se/list/listinfo/curl-library
Etiquette: https://curl.se/mail/etiquette.html
