On 01/12/2012 10:20 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Currently the LXC controller attempts to deal with EOF on a
> tty by spawning a thread todo a edge triggered epoll_wait().
s/todo a edge /to do an edge-/
> This avoids the normal event loop spinning on POLLHUP. There
> is a subtle mistake though - even after seeing POLLHUP on a
> master PTY, it is still perfectly possible & valid to write
> data to the PTY. There is a buffer that can be filled with
> data, even when no client is present.
>
> The second mistake is that the epoll_wait() thread was not
> looking for the EPOLLOUT condition, so when a new client
> connects to the LXC console, it had to explicitly send a
> character before any queued output would appear.
>
> Finally, there was in fact no need to spawn a new thread to
> deal with epoll_wait(). The epoll file descriptor itself
> can be poll()'d on normally.
>
> This patch attempts to deal with all these problems.
>
> - The blocking epoll_wait() thread is replaced by a poll
> on the epoll file descriptor which then does a non-blocking
> epoll_wait() to handle events
> - Even if POLLHUP is seen, we continue trying to write
> any pending output until getting EAGAIN from write.
> - Once write returns EAGAIN, we modify the epoll event
> mask to also look for EPOLLOUT
The description makes sense; now on to the code :)
> + } else if (console->hostEpoll) {
> + VIR_DEBUG("Stop epoll oldContEvents=%x", console->hostEpoll);
> + if (epoll_ctl(console->epollFd, EPOLL_CTL_DEL, console->hostFd, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to add epoll fd"));
s/add/remove/ in the error message
> + } else if (console->contEpoll) {
> + VIR_DEBUG("Stop epoll oldContEvents=%x", console->contEpoll);
> + if (epoll_ctl(console->epollFd, EPOLL_CTL_DEL, console->contFd, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to add epoll fd"));
And again.
> @@ -1103,9 +1163,32 @@ static int lxcControllerMain(int serverFd,
> }
>
> for (i = 0 ; i < nFds ; i++) {
> + consoles[i].epollFd = -1;
> + consoles[i].epollFd = -1;
Why do you have this line twice? Did you mean to initialize epollWatch
instead?
> + consoles[i].hostWatch = -1;
> + consoles[i].contWatch = -1;
> + }
> +
> + for (i = 0 ; i < nFds ; i++) {
> consoles[i].hostFd = hostFds[i];
> consoles[i].contFd = contFds[i];
>
> + if ((consoles[i].epollFd = epoll_create(2)) < 0) {
Should we be using epoll_create1(EPOLL_CLOEXEC)?
Overall, it looks fairly clean; I'm assuming that you actually tested
with it, and I didn't hit any compilation errors. And the man page for
poll definitely recommends that you must plow on until EAGAIN if you use
EPOLLET, so this new code certainly looks cleaner in that regards.
I'm okay giving ACK with nits fixed.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list