search

varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.65k stars 374 forks source link

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

Closed dridi closed 8 months ago

dridi commented 8 months ago

The expectations on FreeBSD are stricter than on Linux.

dridi commented 8 months ago

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

asadsa92 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

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

bsdphk commented 8 months ago

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 commented 8 months ago

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

bsdphk commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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:

dridi commented 8 months ago

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 commented 8 months ago

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.