Hi Cyril,
On Mon, Nov 07, 2016 at 10:38:32PM +0100, Cyril Bonté wrote:
> Hi Baptiste and Willy,
>
> Le 02/11/2016 à 08:19, Baptiste a écrit :
> > Thanks, I completely missed this one. I'm slightly worried that
> > documenting
> > the internal values may cause some people to start to fiddle with
> > the file
> > and cause issues if we ever change certain internal values, but at
> > the same
> > time I don't see any reason for changing these values, so that might
> > be fine.
> >
> >
> >
> > Hi,
> >
> > That's the reason why we "designed" the doc like this :)
> > (and dev should now think to update the doc each time they change the
> > flags).
>
> I perfectly understand the goal. But now the values are publicly exposed, I
> tend to think they should be documented and as stable as possible (I may be
> wrong but I compare it to /proc, where the documentation shouldn't ask
> admins to look at the kernel sources to see which value applies). They are
> kind of an API, now.
>
> Shouldn't we add a reminder in the sources so that the documentation is
> updated as soon as new values are added ? Maybe it can be a compromise.
I tend to agree with you but the problem is that the purpose of the exposure
has never been for these values to be modified nor reused but to be reloaded
between two instances of the process.
I think that what needs to be done now is to use your documentation and
modify the code to explicitly map these values to the internal flags so
that we can continue to modify the internal API as needed without having
to care about the public exposure. We've done the same a long time ago
with the polling flags, that's even why we have this ugly construct in
ev_epoll.c :
if (EPOLLIN == FD_POLL_IN && EPOLLOUT == FD_POLL_OUT &&
EPOLLPRI == FD_POLL_PRI && EPOLLERR == FD_POLL_ERR &&
EPOLLHUP == FD_POLL_HUP) {
n = e & (EPOLLIN|EPOLLOUT|EPOLLPRI|EPOLLERR|EPOLLHUP);
}
else {
n = ((e & EPOLLIN ) ? FD_POLL_IN : 0) |
((e & EPOLLPRI) ? FD_POLL_PRI : 0) |
((e & EPOLLOUT) ? FD_POLL_OUT : 0) |
((e & EPOLLERR) ? FD_POLL_ERR : 0) |
((e & EPOLLHUP) ? FD_POLL_HUP : 0);
}
In short : we know they are the same, but just in case they are not
anymore, let's map them by hand. We don't have this performance constraint
on server state reload so we can simply drop the if and take the equivalent
of the second block above. This way we'll have a real public API that takes
no risk being exposed.
I don't know if anyone is interested in working on this, as I still have
quite a number of other things to review before the release. However if
anyone wants to work on this, please tell me before starting, I'm finishing
to polish a few patches from Baptiste that touch this area a little bit by
adding an extra admin flag.
Cheers,
Willy
