[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

On 10/20/2011 08:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange 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


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

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);


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.

+    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,
+                          NULL)<  0) {
+        lxcError(VIR_ERR_INTERNAL_ERROR, "%s",
+                 _("Unable to watch signal socket"));


ACK with nits fixed.

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]