[libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
On Wed, May 07, 2008 at 12:47:40PM -0700, Dave Leskovec wrote: Daniel Veillard wrote: On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote: [...] -close(vm-parentTty); +//close(vm-parentTty); close(vm-containerTtyFd); if we really don't need this anymore just remove it, if you have doubts then maybe this should be clarified. In any case let's stick to old style comments /* ... */ That shouldn't be commented out. I've restored it in the attached updated patch. Sounds fine then +1, just push it :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote: This patch changes the lxc tty forwarding process to use epoll instead of poll. This is done to avoid a cpu consuming loop when a user disconnects from the container console. During some testing, we found that when the slave end of a tty is closed, calls to poll() on the master end will return immediately with POLLHUP until the slave end is opened again. The result of this is that if you connect to the container console using virsh and then ctrl-] out of it, the container tty process will chew up the processor handling POLLHUP. This event can't be disabled regardless of what is set in the events field. This can be avoided by using epoll. When used in edge triggered mode, you see the initial EPOLLHUP but will not see another one until someone connects and then disconnects from the console again. This also drove some changes into how the regular input data is handled. Once an EPOLLIN is seen from an fd, another one will not be surfaced until all data has been read from the file (EAGAIN is returned by read). Sounds fine in principle but i have a couple of questions with the patch +#include stdbool.h err ... what is that ? looks like a linux specific header, do we really need this ? epoll is linux specific I think but #include sys/epoll.h should be sufficient no ? [...] -close(vm-parentTty); +//close(vm-parentTty); close(vm-containerTtyFd); if we really don't need this anymore just remove it, if you have doubts then maybe this should be clarified. In any case let's stick to old style comments /* ... */ Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
On Wed, May 07, 2008 at 08:25:58AM -0400, Daniel Veillard wrote: On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote: This patch changes the lxc tty forwarding process to use epoll instead of poll. This is done to avoid a cpu consuming loop when a user disconnects from the container console. During some testing, we found that when the slave end of a tty is closed, calls to poll() on the master end will return immediately with POLLHUP until the slave end is opened again. The result of this is that if you connect to the container console using virsh and then ctrl-] out of it, the container tty process will chew up the processor handling POLLHUP. This event can't be disabled regardless of what is set in the events field. This can be avoided by using epoll. When used in edge triggered mode, you see the initial EPOLLHUP but will not see another one until someone connects and then disconnects from the console again. This also drove some changes into how the regular input data is handled. Once an EPOLLIN is seen from an fd, another one will not be surfaced until all data has been read from the file (EAGAIN is returned by read). Sounds fine in principle but i have a couple of questions with the patch +#include stdbool.h err ... what is that ? looks like a linux specific header, do we really need this ? epoll is linux specific I think but #include sys/epoll.h should be sufficient no ? It is kinda academic whether epool is linux specific - the entire LXC driver is Linux specific, so you're not compiling it on Solaris/Windows anyway. So any Linux-isms in LXC driver code is fine Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
Daniel Veillard [EMAIL PROTECTED] wrote: On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote: Sounds fine in principle but i have a couple of questions with the patch +#include stdbool.h err ... what is that ? looks like a linux specific header, do we really need this ? epoll is linux specific I think but #include sys/epoll.h should be sufficient no ? stdbool.h is not Linux specific. It's the C99-specified header that provides e.g,. the bool type. Good C code has been able to use the bool type portably (at least through autoconf/gnulib-provided insulation) for many years. These days you rarely need the compatibility shims, since nearly everyone has a c99-compliant compiler. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
On Wed, May 07, 2008 at 03:45:46PM +0200, Jim Meyering wrote: Daniel Veillard [EMAIL PROTECTED] wrote: On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote: Sounds fine in principle but i have a couple of questions with the patch +#include stdbool.h err ... what is that ? looks like a linux specific header, do we really need this ? epoll is linux specific I think but #include sys/epoll.h should be sufficient no ? stdbool.h is not Linux specific. It's the C99-specified header that provides e.g,. the bool type. Good C code has been able to use the bool type portably (at least through autoconf/gnulib-provided insulation) for many years. These days you rarely need the compatibility shims, since nearly everyone has a c99-compliant compiler. Okay, first time I see it, didn't found it in the standard path, and the man page about it was looking suspicious to me :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re: [Libvir] [PATCH] lxc: loop in tty forwarding process
Daniel Veillard wrote:
On Tue, May 06, 2008 at 12:51:08AM -0700, Dave Leskovec wrote:
[...]
-close(vm-parentTty);
+//close(vm-parentTty);
close(vm-containerTtyFd);
if we really don't need this anymore just remove it, if you have doubts then
maybe this should be clarified. In any case let's stick to old style comments
/* ... */
That shouldn't be commented out. I've restored it in the attached updated
patch.
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
---
src/lxc_driver.c | 172 ---
1 file changed, 128 insertions(+), 44 deletions(-)
Index: b/src/lxc_driver.c
===
--- a/src/lxc_driver.c 2008-05-05 23:21:56.0 -0700
+++ b/src/lxc_driver.c 2008-05-07 11:04:38.0 -0700
@@ -26,9 +26,10 @@
#ifdef WITH_LXC
#include fcntl.h
-#include poll.h
+#include sys/epoll.h
#include sched.h
#include sys/utsname.h
+#include stdbool.h
#include string.h
#include sys/types.h
#include termios.h
@@ -552,7 +553,7 @@
int rc = -1;
char tempTtyName[PATH_MAX];
-*ttymaster = posix_openpt(O_RDWR|O_NOCTTY);
+*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK);
if (*ttymaster 0) {
lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
_(posix_openpt failed: %s), strerror(errno));
@@ -593,75 +594,155 @@
}
/**
+ * lxcFdForward:
+ * @readFd: file descriptor to read
+ * @writeFd: file desriptor to write
+ *
+ * Reads 1 byte of data from readFd and writes to writeFd.
+ *
+ * Returns 0 on success, EAGAIN if returned on read, or -1 in case of error
+ */
+static int lxcFdForward(int readFd, int writeFd)
+{
+int rc = -1;
+char buf[2];
+
+if (1 != (saferead(readFd, buf, 1))) {
+if (EAGAIN == errno) {
+rc = EAGAIN;
+goto cleanup;
+}
+
+lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(read of fd %d failed: %s), readFd, strerror(errno));
+goto cleanup;
+}
+
+if (1 != (safewrite(writeFd, buf, 1))) {
+lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(write to fd %d failed: %s), writeFd, strerror(errno));
+goto cleanup;
+}
+
+rc = 0;
+
+cleanup:
+return rc;
+}
+
+typedef struct _lxcTtyForwardFd_t {
+int fd;
+bool active;
+} lxcTtyForwardFd_t;
+
+/**
* lxcTtyForward:
* @fd1: Open fd
* @fd1: Open fd
*
* Forwards traffic between fds. Data read from fd1 will be written to fd2
- * Data read from fd2 will be written to fd1. This process loops forever.
+ * This process loops forever.
+ * This uses epoll in edge triggered mode to avoid a hard loop on POLLHUP
+ * events when the user disconnects the virsh console via ctrl-]
*
* Returns 0 on success or -1 in case of error
*/
static int lxcTtyForward(int fd1, int fd2)
{
int rc = -1;
-int i;
-char buf[2];
-struct pollfd fds[2];
-int numFds = 0;
-
-if (0 = fd1) {
-fds[numFds].fd = fd1;
-fds[numFds].events = POLLIN;
-++numFds;
+int epollFd;
+struct epoll_event epollEvent;
+int numEvents;
+int numActive = 0;
+lxcTtyForwardFd_t fdArray[2];
+int timeout = -1;
+int curFdOff = 0;
+int writeFdOff = 0;
+
+fdArray[0].fd = fd1;
+fdArray[0].active = false;
+fdArray[1].fd = fd2;
+fdArray[1].active = false;
+
+/* create the epoll fild descriptor */
+epollFd = epoll_create(2);
+if (0 epollFd) {
+lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(epoll_create(2) failed: %s), strerror(errno));
+goto cleanup;
}
-if (0 = fd2) {
-fds[numFds].fd = fd2;
-fds[numFds].events = POLLIN;
-++numFds;
+/* add the file descriptors the epoll fd */
+memset(epollEvent, 0x00, sizeof(epollEvent));
+epollEvent.events = EPOLLIN|EPOLLET;/* edge triggered */
+epollEvent.data.fd = fd1;
+epollEvent.data.u32 = 0;/* fdArray position */
+if (0 epoll_ctl(epollFd, EPOLL_CTL_ADD, fd1, epollEvent)) {
+lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(epoll_ctl(fd1) failed: %s), strerror(errno));
+goto cleanup;
}
-
-if (0 == numFds) {
-DEBUG0(No fds to monitor, return);
+epollEvent.data.fd = fd2;
+epollEvent.data.u32 = 1;/* fdArray position */
+if (0 epoll_ctl(epollFd, EPOLL_CTL_ADD, fd2, epollEvent)) {
+lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(epoll_ctl(fd2) failed: %s), strerror(errno));
goto cleanup;
}
while (1) {
-if ((rc = poll(fds, numFds, -1)) = 0) {
+/* if active fd's, return if no events, else wait forever */
+timeout = (numActive 0) ? 0 : -1;
+numEvents = epoll_wait(epollFd, epollEvent, 1, timeout);
+if (0 numEvents) {
+if (epollEvent.events EPOLLIN)
