[libvirt] [PATCH 3/7] Rewrite LXC I/O forwarding to use main event loop

[Eric Blake]

On 10/20/2011 08:47 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> The current I/O code for LXC uses a hand crafted event loop
> to forward I/O between the container&  host app, based on
> epoll to handle EOF on PTYs. This event loop is not easily
> extendable to add more consoles, or monitor other types of

s/extendable/extensible/

> file descriptors.
>
> Remove the custom event loop and replace it with a normal
> libvirt event loop. When detecting EOF on a PTY, disable
> the event watch on that FD, and fork off a background thread
> that does a edge-triggered epoll() on the FD. When the FD
> finally shows new incoming data, the thread re-enables the
> watch on the FD and exits.
>
> When getting EOF from a read() on the PTY, the existing code
> would do waitpid(WNOHANG) to see if the container had exited.
> Unfortunately there is a race condition, because even though
> the process has closed its stdio handles, it might still
> exist.
>
> To deal with this the new event loop uses a SIG_CHILD handler
> to perform the waitpid only when the container is known to
> have actually exited.
>
> * src/lxc/lxc_controller.c: Rewrite the event loop to use
>    the standard APIs.
> ---
>   cfg.mk                   |    2 +-
>   src/lxc/lxc_controller.c |  607 ++++++++++++++++++++++++++++++----------------
>   2 files changed, 404 insertions(+), 205 deletions(-)

Big; hopefully I get through it today.

> +
> +static void lxcConsoleEOFThread(void *opaque)
> +{
> +    struct lxcConsoleEOFData *data = opaque;
> +    int ret;
> +    int epollfd = -1;
> +    struct epoll_event event;
> +
> +    VIR_WARN("MOnitor %d", data->fd);

s/MOnitor/Monitor/

should this be VIR_DEBUG instead of VIR_WARN?

> +    if ((epollfd = epoll_create(2))<  0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to create epoll fd"));
> +        goto cleanup;
> +    }

Should we be using epoll_create1(EPOLL_CLOEXEC) instead?

> +static void lxcConsoleIO(int watch, int fd, int events, void *opaque)
> +{
...
> +        if (done>  0)
> +            *len += done;
> +        else {
> +            VIR_WARN("Read fd %d done %d errno %d", fd, (int)done, errno);
> +        }

Use {} on both branches or neither, but not just one branch.

> +error:
> +    virEventRemoveHandle(console->contWatch);
> +    virEventRemoveHandle(console->hostWatch);
> +    console->contWatch = console->hostWatch = -1;
> +    quit = true;
> +    virMutexUnlock(&lock);

Some of your code set 'quit = true' outside the 'lock' mutex; was that 
intentional?

> +    if (virMutexInit(&lock)<  0)
> +        goto cleanup2;
> +
> +    if (pipe(sigpipe)<  0) {

Shouldn't the signal pipe be non-blocking?  I'm not sure if cloexec 
matters, but for consistency, we might as well set it.  In other words, 
should this be:

pipe2(sigpipe, O_CLOEXEC|O_NONBLOCK)

> +    if (virEventAddHandle(sigpipe[0],
> +                          VIR_EVENT_HANDLE_READABLE,
> +                          lxcSignalChildIO,
> +&container,
> +                          NULL)<  0) {
> +        lxcError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                 _("Unable to watch signal socket"));

s/socket/pipe/

ACK with nits fixed.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org