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

Jump to bottom

header api: add curl_easy_header and curl_easy_nextheader #8593

Closed
wants to merge 9 commits into from
Closed

header api: add curl_easy_header and curl_easy_nextheader #8593

wants to merge 9 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 14, 2022 •
edited

Early docs: https://github.com/curl/curl/wiki/get-headers-v2

  • code for the two functions
  • man pages for the functions
  • example showing the API functions in use
  • add test case for curl_easy_header
  • figure out how to deal with CONNECT response headers
  • figure out how to deal with 1xx response headers
  • add test case for curl_easy_header with CONNECT
  • documented as EXPERIMENTAL
  • add test case for curl_easy_header with 1xx
  • add test case for curl_easy_header with trailers
  • add test case for curl_easy_header in a redirect case
  • test case for curl_easy_nextheader
  • make it build fine when HTTP support is disabled
  • use a single malloc per stored header
  • make the function calls able to access headers from multiple responses from the transfer
  • allow curl -w to output header contents with %header{name}
  • supports -w %{header_json} - ouputs all headers as JSON
  • make -w %{header_json} handle duplicate headers
  • add test case for -w %header{name}
  • add test case for -w %{header_json}
  • support/handle psuedo headers
  • skip making a test for psuedo headers (due to lacking test infra)
  • make the API (and -w options) build opt-in while EXPERIMENTAL
  • make the tests run only if the headers API is enabled in the build
  • make sure the docs appear nicely on the website (pending PR)

Remove the wiki page (to keep the docs in a single place) after initial merge

@jay
Copy link
Member

jay commented Mar 14, 2022

does this cover 100 status responses, for example

HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade

@bagder

This comment was marked as resolved.

Sign in to view
@bagder

This comment was marked as resolved.

Sign in to view
@bagder

This comment was marked as outdated.

Sign in to view
@bagder bagder force-pushed the bagder/header-api branch 3 times, most recently from 1f8b020 to ebdd863 Compare March 15, 2022 22:23
bagder added a commit that referenced this pull request Mar 16, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
873e05b 
Closes #8593
bagder added a commit that referenced this pull request Mar 16, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
20abea3 
Closes #8593
@bagder

This comment was marked as resolved.

Sign in to view
bagder added a commit that referenced this pull request Mar 17, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
29dd2fb 
Add test 1940 to 1946 to verify.

Closes #8593
jay
jay reviewed Mar 18, 2022
View reviewed changes
lib/headers.c Show resolved Hide resolved
bagder added a commit that referenced this pull request Mar 18, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
e043b63 
Add test 1940 to 1946 to verify.

Closes #8593
bagder added a commit that referenced this pull request Mar 19, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
98ca41a 
Add test 1940 to 1946 to verify.

Closes #8593
bagder added a commit that referenced this pull request Mar 19, 2022
@bagder
header api: add curl_easy_header and curl_easy_nextheader
cdc086e 
Add test 1940 to 1946 to verify.

Closes #8593
@bagder bagder marked this pull request as ready for review March 19, 2022 21:52
@bagder
Copy link
Member Author

bagder commented Mar 20, 2022

I'm ready to merge this within the next few days. Any comments to take into account before that happens?

@bagder bagder closed this in d1e4a67 Mar 22, 2022
@bagder bagder deleted the bagder/header-api branch March 22, 2022 07:27
@nanonyme
Copy link

Hmm, was this supposed to hide the new experimental ABI by default if --enable-headers-api is not given? Because it doesn't seem it does.

@bagder
Copy link
Member Author

bagder commented Apr 28, 2022

Can you clarify what you're asking?

@nanonyme
Copy link

nanonyme commented Apr 28, 2022 •
edited

This post clearly states that these new functions should not be exposed as they are experimental. Yet it looks based on our ABI checks that they are indeed exposed by default even without the flag to expose them https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/jobs/2386983527 Is this intended behaviour or should I file a bug?

@bagder
Copy link
Member Author

bagder commented Apr 28, 2022

The functions are present even when disabled, but they don't do anything. They have no functionality.

@nanonyme
Copy link

I see. But just exposing them means people may call them. As it was said, the ABI is subject to change up to function signatures so isn't it completely unsafe to keep them there even as no-op functions?

@bagder
Copy link
Member Author

bagder commented Apr 28, 2022

Why would someone call them if they don't do anything? And yes, providing experimental features risks that users don't read our warnings and go ahead and use them anyway. I don't see an easy way to prevent that.

@nanonyme
Copy link

Couldn't you just use USE_HEADERS_API macro to hide them as private symbols?

@dfandrich
Copy link
Contributor

dfandrich commented Apr 28, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay left review comments

Assignees
No one assigned
Labels
libcurl API
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bagder @jay @nanonyme @dfandrich