Skip to content

bufq: change read/write signatures#17396

Closed
icing wants to merge 5 commits intocurl:masterfrom
icing:bufq-signatures
Closed

bufq: change read/write signatures#17396
icing wants to merge 5 commits intocurl:masterfrom
icing:bufq-signatures

Conversation

@icing
Copy link
Contributor

@icing icing commented May 20, 2025

Change the signature of bufq functions from

  • ssize_t Curl_bufq_*(..., CURLcode *err) to
  • CURLcode Curl_bufq_*(..., size_t *pn)

This allows us to write slightly less code and avoids the ssize_t/size_t conversions in many cases. Also, it gets the function in line with all the other send/recv signatures.

Added helper functions in cfilters.h for sending from/receving into a bufq.

Fuzzer now fails to build due to these changes and its testing of the bufq API.

icing added 2 commits June 26, 2025 10:26
Change the `ssize_t Curl_bufq_*(..., CURLcode *err)` functions to use
`CURLcode Curl_bufq_*(..., size_t *pn)` instead. In line with the
send/recv adjustments in connection filters and elsewhere.
@icing icing force-pushed the bufq-signatures branch from d7847fa to ad0fb39 Compare June 26, 2025 09:23
@curl curl deleted a comment from testclutch Jun 26, 2025
@icing icing requested review from cmeister2 June 26, 2025 10:10
@icing icing marked this pull request as ready for review June 26, 2025 10:10
@icing icing requested review from bagder June 26, 2025 10:10
@icing
Copy link
Contributor Author

icing commented Jun 26, 2025

@cmeister2 How would we best merge this change? Disable fuzzing of bufq, merge, then adapt and re-enable?

@cmeister2
Copy link
Contributor

@icing probably the best way forward, yes. Happy to approve PRs on curl-fuzzer to do that.

@icing
Copy link
Contributor Author

icing commented Jun 27, 2025

Once this is merged, I'll adapt the fuzzer_bufq for the change and re-enable it.

@vszakats when you find time, would be nice.

@icing icing requested a review from vszakats June 27, 2025 08:48
@vszakats vszakats closed this in d4983ff Jun 27, 2025
@vszakats
Copy link
Member

LGTM, and merged!

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

Development

Successfully merging this pull request may close these issues.

3 participants

Comments