setopt: use the handler table for protocol name to number conversions #9472

monnerat · 2022-09-11T11:43:13Z

This also returns error CURLE_UNSUPPORTED_PROTOCOL rather than CURLE_BAD_FUNCTION_ARGUMENT when a listed protocol name is not found.

A new schemelen parameter is added to Curl_builtin_scheme() to support this extended use.

Note that disabled protocols are not recognized anymore.

Test programs 1597 and 1911 adapted accordingly.

monnerat · 2022-09-11T12:39:31Z

@bagder: it seems not recognizing disabled protocols is a big problem for the cli tool and the tests.
What do we do ?

Accept this restriction and adapt tests & cli tool to it (your websocket fix to protocol2num goes in this direction).
Ignore any unrecognized word: real errors not detected anymore.
Forget about this change

lib/url.c

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

Hm, I think maybe the first alternative is the best:

Accept this restriction and adapt tests & cli tool to it

I believe I intended for CURLOPT_PROTOCOLS_STR to work that way, as I read the description in the man page that way. The downside with that approach is for applications that would think "we can support these protocols XXXX" and then support them for when the libcurl actually supports it and not support them if they don't...

But if we don't work that way, we also can't detect typos...

monnerat · 2022-09-11T13:08:16Z

Hm, I think maybe the first alternative is the best:

OK, I will change that way. It will probably require several checks and take some time. Turning this PR into a draft now.

src/tool_paramhlp.c

monnerat · 2022-09-13T17:58:54Z

@bagder: with this PR, the cli tool relies on the protocols listed by curl_version_info(). It would therefore require to list also RTMPE, RTMPTE, RTMPS and RTMPTS if available (not yet done in this PR). Any objection ?

bagder · 2022-09-14T09:28:12Z

require to list also RTMPE, RTMPTE, RTMPS and RTMPTS if available

I'm not super happy about that, as they will then end up in the curl -V output etc but at the same time I figure that might be the better solution overall. I mean, it makes sense to treat all schemes the same way consistently.

We could possibly add something special for the curl tool that just doesn't show all of them.

monnerat · 2022-09-14T09:44:08Z

I'm not super happy about that, as they will then end up in the curl -V output

I understand your dilemma. In all cases, the tool should adapt. I'll look at it. Thanks for your reply.

monnerat · 2022-09-14T14:03:39Z

I've added "warts" code snippets to the cli tool to support the library whether it returns RTMP?* protocols or not. Those protocols are NOT returned yet.

bagder

I think this looks very good. Just some minor comments/questions.

lib/setopt.c

bagder · 2022-09-15T10:03:13Z

tests/data/test1401

@@ -91,7 +91,7 @@ int main(int argc, char *argv[])
  curl_easy_setopt(hnd, CURLOPT_FTP_SKIP_PASV_IP, 1L);
 %endif
  curl_easy_setopt(hnd, CURLOPT_TCP_KEEPALIVE, 1L);
-  curl_easy_setopt(hnd, CURLOPT_PROTOCOLS_STR, "http,ftp,file");
+  curl_easy_setopt(hnd, CURLOPT_PROTOCOLS_STR, "http");


This is the only test case verifying CURLOPT_PROTOCOLS_STR with ---libcurl, shouldn't it use more than one protocol ?

We can just make the test require support for those protocols to be present.

Yes, but how to check for 3 protocols in the same test? I didn't find a way to make the program "auto-adaptive" and still match the <verify> data.

In the <client> section you list all features that are required:

<features> http ftp file </features>

Thanks for the hint. I didn't know protocol names were features !

This also returns error CURLE_UNSUPPORTED_PROTOCOL rather than CURLE_BAD_FUNCTION_ARGUMENT when a listed protocol name is not found. A new schemelen parameter is added to Curl_builtin_scheme() to support this extended use. Note that disabled protocols are not recognized anymore. Tests adapted accordingly.

As they are now rejected by the library, take care of not passing disabled protocol names to CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR. Rather than using the CURLPROTO_* constants, dynamically assign protocol numbers based on the order they are listed by curl_version_info(). New type proto_set_t implements prototype bit masks: it should therefore be large enough to accomodate all library-enabled protocols. If not, protocol numbers beyond the bit count of proto_set_t are recognized but "inaccessible": when used, a warning is displayed and the value is ignored. Should proto_set_t overflows, enabled protocols are reordered to force those having a public CURLPROTO_* representation to be accessible. Code has been added to subordinate RTMP?* protocols to the presence of RTMP in the enabled protocol list, being returned by curl_version_info() or not.

Disabled protocols are now handled as if they were unknown. Also update the possible protocol list.

bagder · 2022-09-16T21:30:10Z

Thanks!

monnerat · 2022-09-16T22:07:39Z

Thanks for your help.

monnerat mentioned this pull request Sep 11, 2022

setopt: make protocols2num() work with websockets #9467

Closed

bagder reviewed Sep 11, 2022

View reviewed changes

lib/url.c Show resolved Hide resolved

monnerat marked this pull request as draft September 11, 2022 13:08

monnerat force-pushed the proto2num branch 3 times, most recently from 975b89c to 13290a0 Compare September 12, 2022 16:47

sonatype-lift bot reviewed Sep 12, 2022

View reviewed changes

src/tool_paramhlp.c Outdated Show resolved Hide resolved

monnerat force-pushed the proto2num branch 5 times, most recently from ff7bb63 to c9298ae Compare September 13, 2022 15:10

monnerat marked this pull request as ready for review September 13, 2022 17:51

monnerat force-pushed the proto2num branch from 52a1fe3 to 824ffb4 Compare September 14, 2022 13:53

monnerat force-pushed the proto2num branch from 824ffb4 to 335fe23 Compare September 14, 2022 22:45

bagder reviewed Sep 15, 2022

View reviewed changes

monnerat added 3 commits September 15, 2022 13:30

docs: tell about disabled protocols in CURLOPT_*PROTOCOLS_STR.

ddae59d

Disabled protocols are now handled as if they were unknown. Also update the possible protocol list.

monnerat force-pushed the proto2num branch from 335fe23 to ddae59d Compare September 15, 2022 12:44

bagder approved these changes Sep 16, 2022

View reviewed changes

bagder closed this in 9d51329 Sep 16, 2022

monnerat deleted the proto2num branch September 16, 2022 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setopt: use the handler table for protocol name to number conversions #9472

setopt: use the handler table for protocol name to number conversions #9472

monnerat commented Sep 11, 2022

monnerat commented Sep 11, 2022 •

edited

bagder commented Sep 11, 2022

monnerat commented Sep 11, 2022

monnerat commented Sep 13, 2022

bagder commented Sep 14, 2022

monnerat commented Sep 14, 2022

monnerat commented Sep 14, 2022

bagder left a comment

bagder Sep 15, 2022

monnerat Sep 15, 2022

bagder Sep 15, 2022

monnerat Sep 15, 2022

monnerat Sep 15, 2022

bagder commented Sep 16, 2022

monnerat commented Sep 16, 2022

setopt: use the handler table for protocol name to number conversions #9472

setopt: use the handler table for protocol name to number conversions #9472

Conversation

monnerat commented Sep 11, 2022

monnerat commented Sep 11, 2022 • edited

bagder commented Sep 11, 2022

monnerat commented Sep 11, 2022

monnerat commented Sep 13, 2022

bagder commented Sep 14, 2022

monnerat commented Sep 14, 2022

monnerat commented Sep 14, 2022

bagder left a comment

Choose a reason for hiding this comment

bagder Sep 15, 2022

Choose a reason for hiding this comment

monnerat Sep 15, 2022

Choose a reason for hiding this comment

bagder Sep 15, 2022

Choose a reason for hiding this comment

monnerat Sep 15, 2022

Choose a reason for hiding this comment

monnerat Sep 15, 2022

Choose a reason for hiding this comment

bagder commented Sep 16, 2022

monnerat commented Sep 16, 2022

monnerat commented Sep 11, 2022 •

edited