Re: conn.data considered bad

From: Patrick Monnerat via curl-library <curl-library_at_cool.haxx.se>
Date: Sun, 10 Jan 2021 16:00:41 +0100

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
Received on 2021-01-10