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
API: provide meta-data about all easy options #5365
Conversation
@jeroen, here's a first shot at providing the meta-data that we discussed today. This currently just makes an array with all known options. Does it contain the necessary information you'd like? What kind of API would your dream scenario provide from libcurl to aid your binding work? |
@bagder thanks for picking this up. Maybe it is better to expose this as an iterator rather than an array. I think an array would technically break the ABI when it changes length from version to version? I think ideally, this would give options from the linked version of libcurl, rather than the headers at compile time. For example when the user upgrades libcurl after compiling the bindings, or when the user has a newer version of libcurl that we target on the build server that compiles the bindings. I think my dream scenario would involve the following APIs: typedef enum {
CURLOT_LONG, /* long */
CURLOT_OFF_T, /* curl_off_t */
CURLOT_OBJECT, /* void * */
CURLOT_STRING, /* char * */
CURLOT_SLIST, /* struct curl_slist * */
CURLOT_FUNCTION /* function pointer */
} curl_easytype;
typedef struct {
const char *name;
CURLoption id;
curl_easytype type;
} curl_option_info;
/* Lookup options at runtime */
const curl_option_info curl_option_info_get_by_name (const char *name);
const curl_option_info curl_option_info_get_by_id (CURLoption id);
/* Iterates over available options */
curl_option_info *curl_option_info_next(const curl_option_info *x); Maybe all of these things could be static so that we don't have to worry about free'ing them. I did not entirely invent this of course. Ffmpeg's libav have similar api's for listing codes and filters (ffmpeg may be compiled with any possible combination of hundreds of codes and filters). E.g. the docs for avfilter show: |
Thanks! I like those; simple and easy. I'll get back with an update soon. |
5e05f4f
to
d912612
Compare
This iteration provides this API: typedef enum {
CURLOT_LONG, /* long */
CURLOT_OFF_T, /* curl_off_t */
CURLOT_OBJECT, /* void * */
CURLOT_STRING, /* char * */
CURLOT_SLIST, /* struct curl_slist * */
CURLOT_FUNCTION /* function pointer */
} curl_easytype;
struct curl_easyoption {
const char *name;
CURLoption id;
curl_easytype type;
};
const struct curl_easyoption *curl_easy_option_by_name(const char *name);
const struct curl_easyoption *curl_easy_option_by_id (CURLoption id);
const struct curl_easyoption *
curl_easy_option_next(const struct curl_easyoption *prev); I've been pondering on marking options that pass pointers to callbacks with some special marker instead of just Are there any options that are insufficiently detailed with these provided types? This API can of course still introduce other option types in the future (like the blob one in #5357) |
It may be useful distinguish Exposing this information might allow for better type checking in the bindings. |
I'm a bit worried about the extensibility of this API. There's no safe way to extend curl_easyoption with more fields in the future without potentially breaking clients (especially a client compiled against a newer libcurl but linked at run-time against an older one). There are a few approaches we could take to make that possible, but we should make sure we think about it now. Also, it seems like curl_option_info_next() is only going to be useful if the application can start from the first one, but there's no way to get that. |
Well yes like everything the ABI will be backward compatible, not forward compatible. I don't think that is a concern, the same will hold for any new API or symbol. One should be able to upgrade libcurl and expect things will keep working. Obviously build servers need to be conservative when choosing the target versions of libcurl and OS.
If you pass |
It probably happens more often than you realize that applications are run with an older libcurl than on which they were compiled. It surely can happen and we should make sure that we don't crash such applications when it does. libcurl is for the most part, backwards and forwards compatible, and any new APIs should be, too. |
Right, I didn't explain exactly how to use the functions yet. I figured we should agree somewhat first before I spend time on that @dfandrich 's comment:
We can fix that by for example
@jeroen's comment:
Or even "bitmask" which is also used for some. Not a bad idea. The only little thing that it made me think of, is that |
In order to generate the |
You are right. Perhaps we could treat boolean as a special case of enum with values 0 and 1. But if the API always provides the current correct type to the bindings, there shouldn't be any issue I think? The bindings lookup the type for CURLOPT_BOOLEAN in the current version of libcurl at runtime so it doesn't matter if this has changed from previous versions? What I am really after of course is some way to do input validation for the option values that the user is trying to set. Another way to go about this is if you would include a field (e.g. |
On Sun, May 10, 2020 at 05:14:39AM -0700, Jeroen Ooms wrote:
But if the API always provides the current correct type to the bindings, there
shouldn't be any issue I think? The bindings lookup the type for
CURLOPT_BOOLEAN in the current version of libcurl at runtime so it doesn't
matter if this has changed from previous versions?
Bindings for statically-typed languages might not be so flexible. If an
application is written to store a value in a boolean" type and it gets changed
to an enumerated type or an integer in a future release, it's a breaking
change.
|
ping @weltling, as someone who's worked with the PHP/CURL binding. Is this kind of meta-data something you or your fellow PHP peeps think would be good in your work and is there anything we should improve? |
I'm a little afraid of letting this meta information include details that either second-guesses what the code will do (and thus introducing duplicated logic that might not even be a perfect duplicate) or doing something that steps on the API/ABI promises or allowances we have. For this reason, I don't think this meta-data should contain range information. It will make it error-prone and complicated since the range might not be a simple one that is the same for everyone. If we say an option is a boolean in one version, someone might then do something that will depend on the option being a boolean and thus break if we change it to long in the next release - even if we document that we can do it and that there's this risk. Upsetting users or tricking them into making mistakes is never good. But sure, there's quite a large number of long-accepting options that really are booleans... can we do it in a way that doesn't mislead anyone? I think a binding is better off letting libcurl do the range and value checks and return the proper return code when reaching outside the boundaries. |
Edit: I guess I have just basically duplicated #5365 (comment) ;) What I'm missing (well - it's more like a "I wish I had") is an indication that a particular option is meant to be a constant / set of flags. It doesn't have to go into the details but once I have a failing tests for my wrapper it would be beneficial to know if I can just put another "long" option there or do I have to actually create a type. |
Based on @jzakrzewski's comment I took a pass over all existing
(did I miss any?) These are options for which a user really cannot easily guess what values to pass to them without reading the docs and using the supplied extra symbols for those values. Would it be beneficial to mark such options specially as accepting |
What about e.g. https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html ?
Yes that would be helpful. Then we can do something in the bindings that distinguishes this to make sure the user understands that this is not a regular number. |
If my list is correct you're missing:
This is at least what is implemented as some enums/flags in the piece of software I maintain. It may not be exactly what curl regards as such. |
Man, I wasn't very good at this! Your three last ones are however strings: Hang on for an updated take that exposes this info. |
0fb8e55
to
2e4847f
Compare
Now it provides these types: typedef enum {
CURLOT_LONG, /* long (a range of values) */
CURLOT_VALUES, /* long (a defined set or bitmask) */
CURLOT_OFF_T, /* curl_off_t */
CURLOT_OBJECT, /* void * */
CURLOT_STRING, /* char * */
CURLOT_SLIST, /* struct curl_slist * */
CURLOT_FUNCTION /* function pointer */
} curl_easytype; |
Yeah, I just maintain some comlicated piece of software that exposes many curl options to the users. Almost like a language binding. We've mapped some string options to enumerations, so when I pulled the definitions I got the extra stuff also ;)
I'd be happy with that :) |
@jzakrzewski where do you maintain the list of string options to enumerations, I would be interested to steal those. |
@jeroen Unfortunately in a bit of two-decades-old proprietary spaghetti c++. If that was just a mapping I'd be happy to share it... |
Now there's also a typedef enum {
CURLOT_LONG, /* long (a range of values) */
CURLOT_VALUES, /* (a defined set or bitmask) */
CURLOT_OFF_T, /* curl_off_t (a range of values) */
CURLOT_OBJECT, /* pointer (void *) */
CURLOT_STRING, /* (char * to zero terminated buffer) */
CURLOT_SLIST, /* (struct curl_slist *) */
CURLOT_CBPTR, /* (void * passed as-is to a callback) */
CURLOT_FUNCTION /* function pointer */
} curl_easytype; |
674c62a
to
bbd111d
Compare
We still don't have enough information here to generate
Worth providing? |
I'm not sure, the |
BTW, there's a new type setopts coming in #5357: |
Ack yes, that was a bug. Should be fixed in master now. Thanks!
|
a12d16b
to
f0da98d
Compare
I also tested the |
I see you have added the ability to the configure script to disable this API. Is there a macro in the public headers that I can use in the bindings to test if this API is available? So that we can do something like: #include <curl/curl.h>
#ifdef CURL_HAVE_GETOPTIONS
/* code using new api */
#else
/* Fallback implementation here */
#endif Or would there be a better way to detect the API? I cannot use autoconf feature-testing in the R bindings. |
@weltling I'm interested in how you are checking the features at compile time. Can you point me to the code that does this? |
@jeroen, I don't think there is such code yet, but it might be used to make PHP's |
@jeroen wrote:
No. We have a bunch of existing ways to strip out functionality from curl that isn't detectable by users and this would just be another one in that colllection. The users who typically want these things disabled are those who want minimum footprint versions in for example embedded devices and presumably not using any bindings and in control of their own environment. If you run a libcurl with this API disabled, it won't expose any options at all and then I figure it makes sense that there are no options provided or offered. @cmb69 wrote:
Of course. But we can't let that stand in the way for us improving things (and it can possibly work as a motivator for users to upgrade to later versions). How do we want to do this for the next 20 years to make things smoother for everyone involved - at least once they upgrade into a version that supports it. |
f0da98d
to
3d54a17
Compare
3d54a17
to
5f3ffd3
Compare
const struct curl_easyoption *curl_easy_option_by_name(const char *name); const struct curl_easyoption *curl_easy_option_by_id (CURLoption id); const struct curl_easyoption * curl_easy_option_next(const struct curl_easyoption *prev); The purpose is to provide detailed enough information to allow for example libcurl bindings to get option information at run-time about what easy options that exist and what arguments they expect. Closes #5365
5f3ffd3
to
82fb7cc
Compare
const struct curl_easyoption *curl_easy_option_by_name(const char *name); const struct curl_easyoption *curl_easy_option_by_id (CURLoption id); const struct curl_easyoption * curl_easy_option_next(const struct curl_easyoption *prev); The purpose is to provide detailed enough information to allow for example libcurl bindings to get option information at run-time about what easy options that exist and what arguments they expect. Closes #5365
82fb7cc
to
c2a396e
Compare
const struct curl_easyoption *curl_easy_option_by_name(const char *name); const struct curl_easyoption *curl_easy_option_by_id (CURLoption id); const struct curl_easyoption * curl_easy_option_next(const struct curl_easyoption *prev); The purpose is to provide detailed enough information to allow for example libcurl bindings to get option information at run-time about what easy options that exist and what arguments they expect. Closes #5365
c2a396e
to
0d3a20a
Compare
Any final words or suggestions, or are we ready to see this merged? |
const struct curl_easyoption *curl_easy_option_by_name(const char *name); const struct curl_easyoption *curl_easy_option_by_id (CURLoption id); const struct curl_easyoption * curl_easy_option_next(const struct curl_easyoption *prev); The purpose is to provide detailed enough information to allow for example libcurl bindings to get option information at run-time about what easy options that exist and what arguments they expect. Assisted-by: Jeroen Ooms Closes #5365
To allow disabling of the curl_easy_option APIs in a build.
0d3a20a
to
85d51af
Compare
To allow disabling of the curl_easy_option APIs in a build. Closes #5365
Why were the AppVeyor failures on this PR ignored? This error is now showing up in all recent builds since this was merged: Update: Just saw that @bagder is already working on it via #5877. Please consider the AppVeyor CI stable at the moment and take a closer look at CI failures in the future. The only unstable CI at the moment is the Windows CI on Azure Pipelines. |
Our CI set is virtually never stable these days (all-green builds are very rare), which makes it too easy to overlook occasional red builds. I have no excuse in this case, I simply must've overlooked that one. |
This is a generated table with information about all existing options
that curl_easy_setopt() might accept. The list contains the option name,
the option value and the option type.
The purpose is to provide detailed enough information to allow for
example libcurl bindings to get option information at run-time about
what options that exist and what of argument they expect.