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

version: add a feature names array to curl_version_info_data #9583

Closed
wants to merge 3 commits into from

Conversation

monnerat
Copy link
Contributor

@monnerat monnerat commented Sep 24, 2022

New field feature_names contains a pointer to a null-terminated sorted array of feature names. Bitmask field features is deprecated.

This will allow us to support more than 32 features.

Whether the strcasecmp4sort() procedure should be moved to strcase.c is questionable.

@monnerat monnerat marked this pull request as draft Sep 24, 2022
lib/version.c Show resolved Hide resolved
@monnerat monnerat force-pushed the feature-strings branch 5 times, most recently from 3adef61 to 34073e8 Compare Sep 25, 2022
@monnerat monnerat marked this pull request as ready for review Sep 26, 2022
@monnerat monnerat force-pushed the feature-strings branch 3 times, most recently from 4eaf738 to f9dfa20 Compare Sep 27, 2022

\fIfeature_names\fP is a pointer to an array of string pointers, containing the
names of the features that libcurl supports. The array is terminated by a NULL
entry.
Copy link
Member

@bagder bagder Sep 27, 2022

Choose a reason for hiding this comment

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

This mentions that features is deprecated, but the man page still only documents the bits in that bitmask. It says nothing about the potential names in feature_names that users of this function probably are very interested in. The list above should probably list both?

Copy link
Contributor Author

@monnerat monnerat Sep 27, 2022

Choose a reason for hiding this comment

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

Yes, I can do it. Note this means the man page will still have to be updated when a new feature is introduced.

Copy link
Member

@bagder bagder Sep 27, 2022

Choose a reason for hiding this comment

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

That was always the case. How is a user supposed to know what the feature means if it isn't documented?

Copy link
Contributor Author

@monnerat monnerat Sep 27, 2022

Choose a reason for hiding this comment

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

As features is deprecated, do you expect me to remove the bit definitions in the doc ?

Copy link
Member

@bagder bagder Sep 27, 2022

Choose a reason for hiding this comment

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

I think it is way too early for that. The documentation also needs to work as a resource for people reading existing code, and there will be code using these flags for the foreseeable future and then the documentation needs to describe them.

Copy link
Contributor Author

@monnerat monnerat Sep 27, 2022

Choose a reason for hiding this comment

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

I think it is way too early for that.

Then the doc should be ready now !

libcurl was built with support for Unix domain sockets.
.IP zstd
supports HTTP zstd content encoding using zstd library.
.RE
Copy link
Member

@bagder bagder Sep 28, 2022

Choose a reason for hiding this comment

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

This is much better, but now you've duplicated the documentation for each feature, and they're not duplicated identically. I suggest we instead list each feature with both name and bitmask define name, and have one explanation for it.

Copy link
Contributor Author

@monnerat monnerat Sep 28, 2022

Choose a reason for hiding this comment

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

Can I drop descriptions of CURL_VERSION_CONV and CURL_VERSION_KERBEROS4? They're obsolete and have no associated name. Reason: I want to label descriptions by names and these are not named! Or do I associate a dummy name that can never occur :-( ?

Copy link
Contributor Author

@monnerat monnerat Sep 28, 2022

Choose a reason for hiding this comment

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

each feature with both name and bitmask define name, and have one explanation for it.

Done.
(I wish man had a built-in .TABLE macro!)

Copy link
Contributor Author

@monnerat monnerat Sep 28, 2022

Choose a reason for hiding this comment

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

Oops! It seems the new documentation format causes test 1177 to fail :-(

@monnerat monnerat force-pushed the feature-strings branch 4 times, most recently from d777418 to f3ce058 Compare Sep 29, 2022
@jay jay added the libcurl API label Oct 2, 2022
@monnerat monnerat force-pushed the feature-strings branch 2 times, most recently from 5ec6880 to 5d08759 Compare Oct 29, 2022
Copy link
Member

@bagder bagder left a comment

If libcurl returns more/other features in its name list than the list in tool_libinfo.c knows about, I assume they will still get output fine?

@@ -106,6 +110,9 @@ typedef struct {
/* when 'age' is CURLVERSION_TENTH or higher (>= 7.77.0), the members
below exist */
const char *gsasl_version; /* human readable string. */
/* when 'age' is CURLVERSION_ELEVENTH or higher (>= 7.86.0), the members
Copy link
Member

@bagder bagder Nov 8, 2022

Choose a reason for hiding this comment

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

7.87.0 now!!

Copy link
Contributor Author

@monnerat monnerat Nov 8, 2022

Choose a reason for hiding this comment

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

Time past soooo fast !

@@ -1097,6 +1097,7 @@ CURLUSESSL_CONTROL 7.17.0
CURLUSESSL_NONE 7.17.0
CURLUSESSL_TRY 7.17.0
CURLVERSION_EIGHTH 7.72.0
CURLVERSION_ELEVENTH 7.86.0
Copy link
Member

@bagder bagder Nov 8, 2022

Choose a reason for hiding this comment

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

7.87.0

Copy link
Contributor Author

@monnerat monnerat Nov 8, 2022

Choose a reason for hiding this comment

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

Idem

{"Kerberos", NULL, CURL_VERSION_KERBEROS5},
{"Largefile", NULL, CURL_VERSION_LARGEFILE},
{"libz", &feature_libz, CURL_VERSION_LIBZ},
{"MIME", NULL, 0},
Copy link
Member

@bagder bagder Nov 8, 2022

Choose a reason for hiding this comment

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

This is not an actual feature is it?

Copy link
Contributor Author

@monnerat monnerat Nov 8, 2022

Choose a reason for hiding this comment

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

No it isn't. This is a leftover from local tests. Does not harm, but useless. Removed now.

@monnerat
Copy link
Contributor Author

monnerat commented Nov 9, 2022

All your remarks taken into account now. Thanks for pointing on them.

If libcurl returns more/other features in its name list than the list in tool_libinfo.c knows about, I assume they will still get output fine?

Yes. the feature list is taken directly from the name array:

curl/src/tool_help.c

Lines 177 to 182 in f8b8dc4

if(feature_names[0]) {
printf("Features:");
for(builtin = feature_names; *builtin; ++builtin)
printf(" %s", *builtin);
puts(""); /* newline */
}

In case an old library does not provide the feature array, the later is built by the tool from the flags:

curl/src/tool_libinfo.c

Lines 160 to 170 in f8b8dc4

if(curlinfo->age >= CURLVERSION_ELEVENTH && curlinfo->feature_names)
feature_names = curlinfo->feature_names;
else {
const struct feature_name_presentp *p;
const char **cpp = fnames;
for(p = maybe_feature; p->feature_name; p++)
if(curlinfo->features & p->feature_bitmask)
*cpp++ = p->feature_name;
*cpp = NULL;
}

monnerat added 3 commits Nov 11, 2022
Field feature_names contains a null-terminated sorted array of feature
names. Bitmask field features is deprecated.

Documentation is updated. Test 1177 and tests/version-scan.pl updated to
match new documentation format and extended to check feature names too.
If the run-time libcurl is too old to support feature names, the name
array is created locally from the bit masks. This is the only sequence
left that uses feature bit masks.
@bagder bagder closed this in e780aae Nov 14, 2022
bagder pushed a commit that referenced this pull request Nov 14, 2022
If the run-time libcurl is too old to support feature names, the name
array is created locally from the bit masks. This is the only sequence
left that uses feature bit masks.

Closes #9583
@bagder
Copy link
Member

bagder commented Nov 14, 2022

Thanks!

@monnerat monnerat deleted the feature-strings branch Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants