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

v1p: Fix invalid poll() usage with a new VTIM_poll() #4050

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

dridi
Copy link
Member

@dridi dridi commented Feb 1, 2024

The expectations on FreeBSD are stricter than on Linux.

@dridi
Copy link
Member Author

dridi commented Feb 1, 2024

Also, POSIX documents -1 and only that to disable the timeout in poll(2).

@asadsa92
Copy link
Contributor

asadsa92 commented Feb 1, 2024

Could we maybe put it in vtcp.c instead? As we already got VTCP_read() there.
Also note that we got a poll() inside of VTCP_read().

But, an argument can be made for neither to be in the "correct" place.

@dridi
Copy link
Member Author

dridi commented Feb 1, 2024

But, an argument can be made for neither to be in the "correct" place.

I totally agree, I wasn't sure where it belonged, but VTCP could be a better home for this one.

@dridi
Copy link
Member Author

dridi commented Feb 1, 2024

Also note that we got a poll() inside of VTCP_read().

I was planning to review out poll usage once that is merged.

@bsdphk
Copy link
Contributor

bsdphk commented Feb 2, 2024

Tested, works on FreeBSD, but:

It feels somewhat unsanitary to put a wrapper for poll(2) in VTIM.

A int VTIM_poll_timeout(vtim_dur) would be cleaner.

@dridi
Copy link
Member Author

dridi commented Feb 2, 2024

I actually moved it to VTCP_poll() and forgot to push the change to my branch.

@bsdphk
Copy link
Contributor

bsdphk commented Feb 2, 2024

IMO VTCP_poll() has nothing to do with TCP, apart from working on TCP socket fds...

I still think we should start with a int VTIM_poll_timeout(vtim_dur) and unless it has some TCP specific handling, I dont see a need for VTCP_poll() at all ?

@dridi
Copy link
Member Author

dridi commented Feb 2, 2024

Ack, I will amend the branch and merge it once ready.

FWIW I find it a bad fit for all the places I considered: VTIM, VTCP, VUS, VFIL, VEV and Waiter.

@asadsa92
Copy link
Contributor

asadsa92 commented Feb 2, 2024

I agree with not going VTCP, but why not just go for VTIM_poll(vtim_dur); ?
The timeout is implicit, both the name poll and the duration parameter.

EDIT: or if having timeout in the name is desirable then go with:

  • VTIM_timeout(struct pollfd *fds, int nfds, vtim_dur tmo); ?

@dridi
Copy link
Member Author

dridi commented Feb 2, 2024

I agree with not going VTCP, but why not just go for VTIM_poll(vtim_dur); ?

Obviously I'm biased, this is what I picked initially... But I guess we could also have a VTIM_poll_deadline(vtim_real). Going right away with VTIM_poll_timeout() would make both explicit I suppose.

@nigoroll
Copy link
Member

nigoroll commented Feb 5, 2024

bugwash: make it a vtim_dur-to-poll-argument conversion function and use sth like poll(fd, n, VTIM_poll_tmo(duration). Can merge without further review.

dridi added 3 commits February 5, 2024 18:02
It takes a vtim_dur and returns a number of milliseconds. NAN means no
timeout and negative values are considered expired deadlines and get a
chance to succeed a non-blocking poll.
Otherwise poll(2) returns EINVAL on FreeBSD when both pipe_timeout and
pipe_task_deadline are disabled.

Fixes varnishcache#4043
@dridi dridi merged commit 17fa386 into varnishcache:master Feb 5, 2024
12 of 14 checks passed
@dridi dridi deleted the vtim_poll branch February 5, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants