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

uvwget sample is bad. #1364

Open
ryutei opened this issue May 31, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@ryutei
Copy link

commented May 31, 2017

Hello.

Recently I read user-guide of libuv. I'm confused by External I/O with polling code. Here is the thing:

uvwget/main.c

typedef struct
 curl_context_s {
    uv_poll_t poll_handle;
    curl_socket_t sockfd;
} curl_context_t;

…

void curl_perform(uv_poll_t *req, int status, int events) {
....
    curl_context_t *context;

    context = (curl_context_t*)req;
...
}

OK, curl_context_t has uv_poll_t as head of member, this is why curl_perform function can get curl_context_t* from cast of (incompatible) uv_poll_t*. But I think this is anti-pattern of C language. It’s not ordinary way.

uv_poll_t is virtually subclass of uv_handle_t, so it has public member 'data'. External library should use 'data' to store own context, instead of misleading pointer cast of uv_poll_t*.

@bnoordhuis bnoordhuis added the doc label May 31, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 31, 2017

Pull requests welcome. We adopted the libuv book wholesale from its author, quirks and all.

Libuv's internal style would be to use a container_of-like macro.

@kroggen

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

Using the data field to store a pointer to an external struct is not always the answer.

If we want that our private struct be released when the request (or handle) is released then using a "baton" would be better.

Check here

@saghul

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

@kroggen As Ben pointed out, container_of is a cleaner approach.

@kroggen

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2017

Yeah, I agree. I didn't write the opposite. I just wrote about batons, that can be used with container_of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.