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

Example code for curl_multi_waitfds does not match code. #15146

Closed
chrisd-work opened this issue Oct 4, 2024 · 2 comments
Closed

Example code for curl_multi_waitfds does not match code. #15146

chrisd-work opened this issue Oct 4, 2024 · 2 comments

Comments

@chrisd-work
Copy link
Contributor

chrisd-work commented Oct 4, 2024

I did this

Followed the example code at: https://curl.se/libcurl/c/curl_multi_waitfds.html

I expected the following

There are two problems with the documentation of this function found at:

https://curl.se/libcurl/c/curl_multi_waitfds.html

Though there are two problems, they are logically related and should be addressed at the same time.

  1. The example code indicates that passing NULL to the second parameter (ufds) is valid.

The documentations example suggests the following code:

    mc = curl_multi_waitfds(multi, NULL, 0, &fd_count);
 
    if(mc != CURLM_OK) {
      fprintf(stderr, "curl_multi_waitfds() failed, code %d.\n", mc);
      break;
    }

However the code for curl_multi_waitfds in lib/multi.c contains the following:

  if(!ufds)
    return CURLM_BAD_FUNCTION_ARGUMENT;

This is the first code after declaring variables in the function. Note that although the documentation is wrong, the behavior indicated by the documentation would be desirable. Notably, a user has now way to know for sure how large to make ufds so having a facility to indicate this is useful.

  1. If a user passes a non-NULL for the second parameter (ufds) , and 0 for the third (size), the forth parameter, the output fd_count does not give the final size.

The same example code seems to indicate that, a user can use a 0 size, to figure out the fd_count they need, testing this however, it doesn't. This behavior is desirable to the user. In fact the recourses left to the user are not optimal. There are two:

  1. Allocate a large enough buffer to accommodate any reasonable fd_count, would need.

The problem with this solution is what is "large enough" there is no way to know.

  1. Allocate say a doubling of the buffer every time that CURLM_OUT_OF_MEMORY is returned.

There are several issues with this:

a) Is CURLM_OUT_OF_MEMORY a result of size not being large enough, or something else?
b) How many doubling are enough to just stop at?
c) It can be inefficient.
d) It burdens user code with ineloquent code.

It is my opinion that the documentation is the desirable state of the code, and the code should be updated to match the documentation. I am willing to make these changes to the libcurl codebase if there is agreement that these changes should be made.

Notable the changes to the code would be to allow the code in the example to work, and add a unit test.

curl/libcurl version

curl 8.10.1, dev

operating system

Ubuntu Linux

@jay
Copy link
Member

jay commented Oct 4, 2024

I agree with both your points. The doc specifically says size of 0 can be used to get the count which would imply to anyone familiar with API calls like this that the normally allocated memory pointer can be NULL in that case.

It is my opinion that the documentation is the desirable state of the code, and the code should be updated to match the documentation. I am willing to make these changes to the libcurl codebase if there is agreement that these changes should be made.

That would be great. Please open a PR and we'll take a look.

/cc @dkarpov1970

@jay jay added the libcurl API label Oct 4, 2024
@bagder
Copy link
Member

bagder commented Oct 4, 2024

This looks like an oversight in the code, yes.

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

No branches or pull requests

6 participants
@bagder @jay @chrisd-work and others