This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: BZ#15819: introduce internal function to ease poll retry with timeout
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Alexandre Oliva <aoliva at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Tue, 9 Dec 2014 11:31:30 -0800 (PST)
- Subject: Re: BZ#15819: introduce internal function to ease poll retry with timeout
- Authentication-results: sourceware.org; auth=none
- References: <orioiiahsf dot fsf at free dot home> <20141113221831 dot D15A62C3B18 at topped-with-meat dot com> <or4mu2zfsf dot fsf at free dot home>
> > The __ treatment is never necessary for local-scope names in internal
> > source files (including headers). It only matters for global names, or
> > uses in installed headers.
>
> I figured names outside the reserved namespace could be used in our
> tests, and then they'd interfere. I guess the reasoning is that, since
> we control the tests too, we can just fix any such conflicts in the
> tests, rigth?
How would they interfere? We're not talking about external linkage names.
> This revised patch fixes both of these issues, and it poisons __poll and
> poll at the end of include/poll-noeintr.h.
We don't have any precedent for using '#pragma poison' in libc. I'm not
particularly against it, but it is something we have not seen before. It
needs comments explaining what it means and why it's there. I think we
should also have a consensus policy on use of this feature, which would be
written up on the wiki.
> --- /dev/null
> +++ b/include/poll-noeintr.h
[...]
> +#ifndef _POLL_EINTR_H
> +#define _POLL_EINTR_H
Make the guard macro match the file name.
> + /* At least CLOCK_REALTIME should always be supported, but
> + if clock_gettime fails for any other reason, the best we
> + can do is to retry with a slightly lower timeout, until
> + we complete without interruption. */
> + timeout--;
> + goto retry_poll;
This could let TIMEOUT go negative, which has the wrong semantics.
You need to cap it at zero.
> + tsend.tv_sec = tscur.tv_sec + timeout / 1000;
> + tsend.tv_nsec = tscur.tv_nsec + timeout % 1000 * 1000000L + 500000;
We really should have some inlines/macros for these canonical time
conversion calculations, rather than repeating them.
Thanks,
Roland