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

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

Closed
wants to merge 3 commits into from

Conversation

monnerat
Copy link
Contributor

@monnerat monnerat commented Sep 11, 2022

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

monnerat commented Sep 11, 2022

@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 Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Sep 11, 2022

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

monnerat commented Sep 11, 2022

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.

@monnerat monnerat marked this pull request as draft Sep 11, 2022
@monnerat monnerat force-pushed the proto2num branch 3 times, most recently from 975b89c to 13290a0 Compare Sep 12, 2022
src/tool_paramhlp.c Outdated Show resolved Hide resolved
@monnerat monnerat force-pushed the proto2num branch 5 times, most recently from ff7bb63 to c9298ae Compare Sep 13, 2022
@monnerat monnerat marked this pull request as ready for review Sep 13, 2022
@monnerat
Copy link
Contributor Author

monnerat commented Sep 13, 2022

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

bagder commented Sep 14, 2022

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

monnerat commented Sep 14, 2022

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

monnerat commented Sep 14, 2022

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.

Copy link
Member

@bagder bagder left a comment

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

lib/setopt.c Show resolved Hide resolved
lib/setopt.c Show resolved Hide resolved
@@ -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");
Copy link
Member

@bagder bagder Sep 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@monnerat monnerat Sep 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bagder bagder Sep 15, 2022

Choose a reason for hiding this comment

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

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

<features>
http
ftp
file
</features>

Copy link
Contributor Author

@monnerat monnerat Sep 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@monnerat monnerat Sep 15, 2022

Choose a reason for hiding this comment

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

Fixed now!

monnerat added 3 commits Sep 15, 2022
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
bagder approved these changes Sep 16, 2022
@bagder
Copy link
Member

bagder commented Sep 16, 2022

Thanks!

@bagder bagder closed this in 9d51329 Sep 16, 2022
@monnerat
Copy link
Contributor Author

monnerat commented Sep 16, 2022

Thanks for your help.

@monnerat monnerat deleted the proto2num branch Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants