multi: add new information extraction methods#17992
multi: add new information extraction methods#17992icing wants to merge 5 commits intocurl:masterfrom
Conversation
vszakats
left a comment
There was a problem hiding this comment.
Nice!
I like how the type is reflected in the enum name. (It seems useful to keep future type names 4 letters, for readability/alignment.)
It crossed my mind to include _info_ in the function names, but since both have _multi_get_ in them, in sync with an existing getter, it's nicely greppable without adding anything.
Returning the value directly makes the new APIs easy to use. The slight downside is no way to return a failure or a non/missing-value (not in a single call). Existing counterpart curl_easy_getinfo() can return two errors: CURLE_UNKNOWN_OPTION, or CURLE_BAD_FUNCTION_ARGUMENT if the handle is NULL. I think in these cases it's just fine to return zero, as done already.
In some many words: This looks good to me!
|
I had at first the variant returning |
|
I'm thinking uint might not be the best type to use because it can be different
sizes on different platforms leading to platform differences. uint64 would be
my suggestion, given that we require 64-bit type everywhere now. That's less
efficient on 32-bit platforms, though, but those are less of an issue these
days, and merely converting an int through this API doesn't add much overhead.
A longer type won't make a difference for these current four info items (you'll
never get near 2^32 handles on a 32-bit platform) but it could make a
difference for items we add in the future.
I concur with the suggestion to include "INFO" in the enums.
|
I chose Maybe there is a way out of this, I am not aware of? Update: the alternative would be to use |
Sounds sensible. Maybe with an alias indicating the semantic |
|
Updated PR to only have the |
|
Made #18004 to address the allocation limit failure of test1 on alpine musl. |
|
A pretty major drawback with not being able to return an error: an application can't figure out if the info it asks for is actually zero (or -1?) when in reality the libcurl version it uses is just not new enough to support that info... |
Always good to use the latest curl...😌 Yeah, I can switch back to the version that returns a |
I think that will be a better API for when we add new ones in the future. |
Changed now. |
| via DoH, for example). | ||
|
|
||
| For the current number of easy handles managed by the multi, use | ||
| *CURLMINFO_XFERS_CURRENT*. |
There was a problem hiding this comment.
I think the documentation for these options should be converted into their own dedicated man pages like they are for curl_easy_getinfo() and others, but I am also okay with doing that in a separate PR after this is initially merged.
There was a problem hiding this comment.
We can do that should it grow or if people come with too many questions regarding those.
Adds `curl_off_t curl_multi_get_offt(CURLM *multi_handle, CURLMinfo_offt info)` to the multi interface with enums: * CURLMINFO_XFERS_CURRENT: current number of transfers * CURLMINFO_XFERS_RUNNING: number of running transfers * CURLMINFO_XFERS_PENDING: number of pending transfers * CURLMINFO_XFERS_DONE: number of finished transfers to read * CURLMINFO_XFERS_ADDED: total number of transfers added, ever Add documentation for functions and info enums. Add use in the curl command line tool to replace two static variables counting the same "from the outside". refs curl#17870
(updated after review by @dfandrich and @vszakats)
multi: add new information extraction method
Adds
curl_off_t curl_multi_get_offt(CURLM *multi_handle, CURLMinfo_offt info)to the multi interface with enums:CURLMINFO_XFERS_CURRENT: current number of transfersCURLMINFO_XFERS_RUNNING: number of running transfersCURLMINFO_XFERS_PENDING: number of pending transfersCURLMINFO_XFERS_DONE: number of finished transfers to readCURLMINFO_XFERS_ADDED: total number of transfers added, everAdd documentation for functions and info enums.
Add use in the curl command line tool to replace two static variables counting the same "from the outside".
refs #17870