[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

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 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]