Repository navigation
setopt: return CURLE_NOT_BUILT_IN for known but disabled options - #23309
JayRGadekar wants to merge 1 commit into
Conversation
When libcurl is built with a feature disabled, curl_easy_setopt() on the options belonging to that feature returned CURLE_UNKNOWN_OPTION, the same as for an option number libcurl has never heard of. The documentation says such options return CURLE_NOT_BUILT_IN, and applications cannot tell a stripped build from an invalid option. Turn CURLE_UNKNOWN_OPTION into CURLE_NOT_BUILT_IN in Curl_vsetopt() when curl_easy_option_by_id() knows the option. An option unknown to libcurl still returns CURLE_UNKNOWN_OPTION. Add test 1923, which sets every known option and fails if any returns CURLE_UNKNOWN_OPTION, whatever the build configuration. Remove the corresponding entry from KNOWN_BUGS. Ref: curl#15472 This change was made with the help of an AI assistant (Claude).
| CURLcode result = setopt_dispatch(data, option, param); | ||
| /* An option that libcurl knows about, but which is left out of this build | ||
| because its feature is disabled, is "not built in" and not "unknown". */ | ||
| if((result == CURLE_UNKNOWN_OPTION) && curl_easy_option_by_id(option)) |
There was a problem hiding this comment.
Isn't this adding an overhead to all setopt calls after this patch?
Even with a small overhead, it adds up, and may not to be worth
the runtime price in many use-cases.
Also, isn't the not-built-in status decided at build-time? If so, is it
perhaps possible to make the decision to return this status at
build-time too, rather than using the runtime lookup this patch
implements?
There was a problem hiding this comment.
Thanks for looking at it. I measured it on a -O2 build: a successful
curl_easy_setopt() takes about 8.5 ns before and about 8.8 ns after, so
the difference is within noise. The lookup only runs when the call would
return CURLE_UNKNOWN_OPTION anyway, so a successful call pays one
comparison and one extra function call. The failing call goes from about
7 ns to about 42 ns.
You are right that this is known at build time. I used the lookup to keep
the diff small, but it also does nothing when libcurl is built with
CURL_DISABLE_GETOPTIONS, where curl_easy_option_by_id() returns NULL.
I can rework it so the decision is made at build time: a switch on the
disabled options, grouped by their CURL_DISABLE_* guard. It is about 180
labels, so a bigger diff. Would you prefer that?
There was a problem hiding this comment.
The trick, I think, is to avoid runtime overhead while also minimizing built-time
branching (and the diff with it) to offer this enhancement. It's also possible there
is no effective way to implement this. My guess is these are the reasons why it's
still on the TODO list.
A build-time solution, if feasible, would also remove the limitation of requiring
CURL_DISABLE_GETOPTION=0.
Curious, are you employing an LLM to make the PR and measurements above?
There was a problem hiding this comment.
The trick, I think, is to avoid runtime overhead while also minimizing built-time branching (and the diff with it) to offer this enhancement. It's also possible there is no effective way to implement this. My guess is these are the reasons why it's still on the TODO list.
Makes sense that there was no forks/prs for this yet
Curious, are you employing an LLM to make the PR and measurements above?
Yes, I am using claude for this and understanding the code myself as I am new to open source contributions and this is my second PR so it really helps me with understanding the bugs and solutions.
2 participants
When libcurl is built without a feature (smtp or proxy support, for
example), curl_easy_setopt() on that feature's options returns
CURLE_UNKNOWN_OPTION, the same as for an option number that does not
exist. The curl_easy_setopt man page says CURLE_NOT_BUILT_IN is returned
for options disabled at build time, and #15472 asks for this (it was
listed in KNOWN_BUGS).
This turns CURLE_UNKNOWN_OPTION into CURLE_NOT_BUILT_IN in
Curl_vsetopt() when curl_easy_option_by_id() knows the option. An option
libcurl does not know at all still returns CURLE_UNKNOWN_OPTION. I removed
the KNOWN_BUGS entry.
Test 1923 sets every known option and fails if any returns
CURLE_UNKNOWN_OPTION. It fails without the change and passes with it.
Tested on Linux (WSL Ubuntu), on a build with many features disabled and
on a full-featured one. On the stripped build, 135 of 312 options returned
CURLE_UNKNOWN_OPTION before and none do now. In the full test suite
everything passes except:
change
change since d8363b7