lib and tool: install large bit mask type when more than 32 protocols #9534

monnerat · 2022-09-18T17:31:38Z

In the library, complete support for more than 32 protocols:

Describe a conditional sequence to define a new protocol bit mask.
Introduce a conditional curl_prot_t definition based on high protocols availability.
Fix a cast in a curl_prot_t assignment.
Remove an outdated comment.

Large prototype bit mask type is also introduced in the cli tool. As there is no way to determine whether the library supports high protocols at compile time, this is unconditional.

Follow-up to cd5ca80.

bagder · 2022-09-18T22:13:24Z

Doesn't this need some precaution or checks for the case where curl_off_t is not larger than unsigned int ? I imagine we could possibly make libcurl work with >32 protocols even if curl_off_t is 32 bits...

monnerat · 2022-09-18T22:47:05Z

Doesn't this need some precaution or checks for the case where curl_off_t is not larger than unsigned int ?

I already had some thinking about it and ended up with the following points:

As it is now in this PR, we'll surely have compile errors on 32-bit curl_off_t platforms if a high protocol is enabled.
If we want to avoid it, what to do in the conditional branch for 32-bit curl_off_t and high protocol enabled? I don't see something useful but a #error, which is also a compile error.
As cited in the previous days' discussions, we are not even sure such 32-bit only platforms currently exist and support curl!

I came to the conclusion that adding several lines in configure and in header files just to generate another error message is not worth it.
Any suggestion?

I imagine we could possibly make libcurl work with >32 protocols even if curl_off_t is 32 bits...

For that, we should have an fd_set-like curl_prot_t and forget about simple C bitwise operators on it, I'm afraid.
If this feature is retained, we better differentiate single protocol and protocol set types as I did in the cli tool, because a non-scalar curl_prot_t field is hardly initialisable statically.

bagder · 2022-09-19T05:36:38Z

forget about simple C bitwise operators on it, I'm afraid

I don't see how we are married to the bitwise operators in the tool code. We could replace that without the same limitations. It doesn't even have to be lightning fast because we dont' do those checks very often.

bagder · 2022-09-19T07:29:18Z

Also, I think you're mixing two completely separate changes in a single PR here. The tool change and library one seem unrelated. Right?

monnerat · 2022-09-19T07:47:48Z

I don't see how we are married to the bitwise operators in the tool code.

I was rather and primary speaking about the library. Your mailing list answer https://curl.se/mail/lib-2022-09/0022.html seems to tell you don't want a non-scalar curl_prot_t in the library..

Also, I think you're mixing two completely separate changes in a single PR here. The tool change and library one seem unrelated. Right?

Yes and no! They are unrelated in terms of code independence. They however are both needed to support >32 protocols.
Explanation: to break the 32-bit barrier and make the sieve PR work again, I had to apply changes both in lib and tool.

bagder · 2022-09-19T08:01:52Z

I was rather and primary speaking about the library

See, we're mixing things here now because the change is in both areas and they are very different territories.

In the libcurl code I think we can stick to only do the new protocols if we are >32 bits - until someone actually shows up that has a system without long long. I suspect they are virtually extinct these days.

monnerat · 2022-09-19T08:21:54Z

In the libcurl code I think we can stick to only do the new protocols if we are >32 bits - until someone actually shows up that has a system without long long. I suspect they are virtually extinct these days.

Yes, I think so too Thus there is no need for a cumbersome test on curl_off_t size in urldata.h. Right ? Those hypothetical platforms will get a compile error anyway.

I imagine we could possibly make libcurl work with >32 protocols even if curl_off_t is 32 bits...

IMO, both quotes above are contradictory.

bagder · 2022-09-19T08:40:58Z

IMO, both quotes above are contradictory.

No they are not. They are two separate ones:

I don't think we currently need to support new protocols for "small" curl_off_t
In a future we might make libcurl work with >32 protocols even if curl_off_t is 32 bits

bagder · 2022-09-19T08:46:56Z

We retain the option (2) because there is now nothing in the API that specifies any particular limit on 32 (nor 64) since we went CURLOPT_PROTOCOLS_STR.

monnerat · 2022-09-19T08:55:31Z

I don't think we currently need to support new protocols for "small" curl_off_t

Right, because we don't have > 32 protocols.

2, In a future we might make libcurl work with >32 protocols even if curl_off_t is 32 bits

That day, a non-scalar curl_prot_t will be needed.

There are then 2 options:

We implement this feature before the next protocol and the latter will then be immediately available on all platforms.
We do not care about "small" platforms at the next protocol commit and wait for someone to moan about a compile error.

Changes in this PR fit option 2.

bagder · 2022-09-19T09:00:17Z

We do not care about "small" platforms at the next protocol commit and wait for someone to moan about a compile error.

The current tool code still makes the tool do assumptions about how many the protocols are and that they will fit in a single scalar variable. It does not know this and there is no guarantee in any API that they will.

Even if we expand that limit (for most systems) with going to curl_off_t.

bagder · 2022-09-19T09:05:49Z

lib/urldata.h

+
+#ifndef PROTO_TYPE_SMALL
+typedef curl_off_t curl_prot_t;
+#else


This adds code and a comment for the future only, and I am generally against that and I am now too. Because most commonly, once that future arrives, some things have changed anyway causing things to get done differently.

How would you then prepare the way for a developer to submit us a PR for a new protocol ?

Adjust that commit to what is needed for the new protocol. I presume that would then remove the comment that says something about the future with actual code for the work in progress.

monnerat · 2022-09-19T09:18:29Z

Even if we expand that limit (for most systems) with going to curl_off_t.

(Speaking about the tool) Yes, but this is yet at least a superset of what is supported in the lib.
The universal alternative should be fully dynamic, probably dealing with string arrays only.
Another important point is the protocol order (for tests purpose): we could move to the alpha order to respect what is returned by curl_version_info() rather than the current CURLPROTO_* order

bagder · 2022-09-19T09:27:27Z

The universal alternative should be fully dynamic, probably dealing with string arrays only.

Not only universal, also the not making assumptions about the number of protocols version. I don't think there's a hurry to fix this, but I think it would be prudent and correct to do it.

Another important point is the protocol order (for tests purpose): we could move to the alpha order to respect what is returned by curl_version_info() rather than the current CURLPROTO_* order

I'm not really following, but yes the protocol supported should be shown alpha-sorted for -V.

monnerat · 2022-09-19T09:33:55Z

Not only universal, also the not making assumptions about the number of protocols version. I don't think there's a hurry to fix this, but I think it would be prudent and correct to do it.

It doesn't really make assumptions: there are of course some limitations (like static array size or dynbuf max length), but protocol numbers are private to the tool and only limited by the proto_t capacity (that means at least 2^32-1). The proto_set_t type limits the number of usable protocols only, not their absolute range in the lib. In particular, if we have disabled low lib-numbered protocols, they "make room" for a high numbered one in the proto_set_t type.

bagder · 2022-09-19T09:38:46Z

In src/tool_libinfo.h this code

#define PROTO_BIT(p) ((p) < PROTO_MAX? (proto_set_t) 1 << (p): \

... is an assumption that there is less than 33 or 65 protocols, depending on size of proto_set_t. Am I wrong?

monnerat · 2022-09-19T09:53:19Z

... is an assumption that there is less than 33 or 65 protocols, depending on size of proto_set_t. Am I wrong?

That's only an internal limitiation due to the proto_set_t type implementation. As I said higher this can be changed but I do not see the need for the tool to have much more capacity than the library. The change in this PR sets it to >= library capacity.

I'm not really following, but yes the protocol supported should be shown alpha-sorted for -V.

-V is already OK. the importance of order for the tests is to have a defined order of protocols listed for matches in the <verify> sections (see test 1401).

bagder · 2022-09-19T10:37:22Z

I do not see the need for the tool to have much more capacity than the library.

The tool simply does not know how many protocols the library supports (without dynamically counting the number curl_version_info returns). The tool implementation of today should be able to run with the library we ship in ten years. Maybe that library supports a different amount of protocols. Therefore, any assumption that the number fits as number of bits within a scalar variable is a gamble.

monnerat · 2022-09-19T10:56:01Z

The tool implementation of today should be able to run with the library we ship in ten years.

do you intend to support >64 protocols in 10 years ?

Therefore, any assumption that the number fits as number of bits within a scalar variable is a gamble.

Yes, but not more than in the current library.

Anyway I agree with you: it should be more scalable. An all-dynamic solution would probably be a luxury, but we should support a large number of protocols via a non-scalar proto_set_t or any other method. A limit would however exist if not all-dynamic.

Just a personal question: why are you so preoccupated by the future of the tool and opposed to advanced prepration of the lib at the same time ? This appears to me as antagonist, although distinct.

bagder · 2022-09-19T11:07:40Z

do you intend to support >64 protocols in 10 years ?

No, I use that to illustrate how we should make the tool only use the knowledge it has, not assume things.

Yes, but not more than in the current library.

The library doesn't assume it knows.

bagder · 2022-09-19T11:08:15Z

why are you so preoccupated by the future

I think we should use the API and not guess what goes beyond it. Future, history and current.

bagder · 2022-09-22T11:50:22Z

@monnerat, after #9546 was merged, what to do want to do with this PR?

monnerat · 2022-09-22T12:02:05Z

what to do want to do with this PR?

Resolve conflict first: the tool change is obsolete and not needed anymore.
Still keep the PR, because it fixes a potential bug in lib/url.c.
Find some way to move the definition of curl_prot_t outside the WS conditionals that satisfies your wish of not designing future things. This to ease new protocols design. Protocols are future but their design is not.

Move the curl_prot_t to its own conditional block. Introduce symbol PROTO_TYPE_SMALL to control it. Fix a cast in a curl_prot_t assignment. Remove an outdated comment. Follow-up to cd5ca80.

bagder · 2022-09-22T21:42:14Z

lib/urldata.h

+#define PROTO_TYPE_SMALL
+
+#ifndef PROTO_TYPE_SMALL
+typedef curl_off_t curl_prot_t;


wouldn't this check be easier and make more sense if reversed?

Like with #ifdef PROTY_TYPE_BIG ...

So that we have a define we set when we cross the 32 boundary and need to use a larger type? Especially since that new protocol will require a 64 bit data type, so going beyond 32 needs to be conditional (at least until we require a 64 bit type).

I don't think so. I started by implementing it as PROTO_TYPE_LARGE, but I noted this is easier to undefine a symbol than to redefine it and I reversed the logic.

#undef can be applied on an undefined symbol: it's just ignored.

#define might redefine an existing symbol only if its value does not change. But this is implementation-dependent, thus we better consider it may not.

When several new protocols (!) will be introduced, they will all need to act on this, so this will be easier to simply #undef the symbol than to redefine it for each protocol.

I know this is future, but we have to prepare for it :-)

Ok, fair enough. We can always change it later if we think of improvements down the line.

bagder · 2022-09-23T06:40:00Z

Thanks!

monnerat · 2022-09-23T09:37:44Z

Thanks for pulling!

bagder reviewed Sep 19, 2022

View reviewed changes

monnerat mentioned this pull request Sep 20, 2022

tool: remove protocol count limitation #9546

Closed

lib: prepare the incoming of additional protocols

709f4d4

Move the curl_prot_t to its own conditional block. Introduce symbol PROTO_TYPE_SMALL to control it. Fix a cast in a curl_prot_t assignment. Remove an outdated comment. Follow-up to cd5ca80.

monnerat force-pushed the proto64 branch from 9e22c51 to 709f4d4 Compare September 22, 2022 15:08

bagder reviewed Sep 22, 2022

View reviewed changes

bagder approved these changes Sep 23, 2022

View reviewed changes

bagder closed this in 91e06e6 Sep 23, 2022

monnerat deleted the proto64 branch September 23, 2022 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib and tool: install large bit mask type when more than 32 protocols #9534

lib and tool: install large bit mask type when more than 32 protocols #9534

monnerat commented Sep 18, 2022

bagder commented Sep 18, 2022

monnerat commented Sep 18, 2022 •

edited

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022 •

edited

bagder Sep 19, 2022

monnerat Sep 19, 2022

bagder Sep 19, 2022

monnerat Sep 22, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022 •

edited

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022 •

edited

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 22, 2022

monnerat commented Sep 22, 2022

bagder Sep 22, 2022

monnerat Sep 22, 2022 •

edited

bagder Sep 23, 2022

bagder commented Sep 23, 2022

monnerat commented Sep 23, 2022

lib and tool: install large bit mask type when more than 32 protocols #9534

lib and tool: install large bit mask type when more than 32 protocols #9534

Conversation

monnerat commented Sep 18, 2022

bagder commented Sep 18, 2022

monnerat commented Sep 18, 2022 • edited

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022 • edited

bagder Sep 19, 2022

Choose a reason for hiding this comment

monnerat Sep 19, 2022

Choose a reason for hiding this comment

bagder Sep 19, 2022

Choose a reason for hiding this comment

monnerat Sep 22, 2022

Choose a reason for hiding this comment

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022 • edited

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022 • edited

bagder commented Sep 19, 2022

monnerat commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 19, 2022

bagder commented Sep 22, 2022

monnerat commented Sep 22, 2022

bagder Sep 22, 2022

Choose a reason for hiding this comment

monnerat Sep 22, 2022 • edited

Choose a reason for hiding this comment

bagder Sep 23, 2022

Choose a reason for hiding this comment

bagder commented Sep 23, 2022

monnerat commented Sep 23, 2022

monnerat commented Sep 18, 2022 •

edited

bagder commented Sep 19, 2022 •

edited

monnerat commented Sep 19, 2022 •

edited

monnerat commented Sep 19, 2022 •

edited

monnerat Sep 22, 2022 •

edited