Roger Pau Monne
2011-Oct-13 11:04 UTC
[Xen-devel] [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1318503503 -7200
# Node ID 72ef13cb4609c97a26efd4931e65c9798fe22572
# Parent 64f17c7e6c33e5f1c22711ae9cbdcbe191c20062
libxl: reimplement buffer for bootloading and drop data if buffer is full.
Implement a buffer for the bootloading process that appends data to the end
until it''s full. Drop output from bootloader if a timeout has occurred
and the buffer is full. Prevents the bootloader from getting stuck when using
ptys with small buffers.
Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>
diff -r 64f17c7e6c33 -r 72ef13cb4609 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c Tue Oct 11 10:26:32 2011 +0200
+++ b/tools/libxl/libxl_bootloader.c Thu Oct 13 12:58:23 2011 +0200
@@ -21,6 +21,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <sys/ioctl.h>
#include "libxl.h"
#include "libxl_internal.h"
@@ -28,7 +29,8 @@
#include "flexarray.h"
#define XENCONSOLED_BUF_SIZE 16
-#define BOOTLOADER_BUF_SIZE 1024
+#define BOOTLOADER_BUF_SIZE 4096
+#define BOOTLOADER_TIMEOUT 1
static char **make_bootloader_args(libxl__gc *gc,
libxl_domain_build_info *info,
@@ -165,10 +167,11 @@ static pid_t fork_exec_bootloader(int *m
*/
static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int
bootloader_fd, int fifo_fd)
{
- int ret;
+ int ret, read_ahead, timeout;
size_t nr_out = 0, size_out = 0;
char *output = NULL;
+ struct timeval wait;
/* input from xenconsole. read on xenconsoled_fd write to bootloader_fd */
int xenconsoled_prod = 0, xenconsoled_cons = 0;
@@ -181,39 +184,64 @@ static char * bootloader_interact(libxl_
fd_set wsel, rsel;
int nfds;
+ /* Set timeout to 1s before starting to discard data */
+ wait.tv_sec = BOOTLOADER_TIMEOUT;
+ wait.tv_usec = 0;
+
if (xenconsoled_prod == xenconsoled_cons)
xenconsoled_prod = xenconsoled_cons = 0;
if (bootloader_prod == bootloader_cons)
bootloader_prod = bootloader_cons = 0;
+ /* Move buffers around to drop already consumed data */
+ if (xenconsoled_cons > 0) {
+ xenconsoled_prod -= xenconsoled_cons;
+ memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons],
xenconsoled_prod);
+ xenconsoled_cons = 0;
+ }
+ if (bootloader_cons > 0) {
+ bootloader_prod -= bootloader_cons;
+ memmove(bootloader_buf, &bootloader_buf[bootloader_cons],
bootloader_prod);
+ bootloader_cons = 0;
+ }
FD_ZERO(&rsel);
FD_SET(fifo_fd, &rsel);
nfds = fifo_fd + 1;
- if (xenconsoled_prod == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE
&& xenconsoled_cons == 0)) {
+ if (xenconsoled_prod < XENCONSOLED_BUF_SIZE) {
FD_SET(xenconsoled_fd, &rsel);
nfds = xenconsoled_fd + 1 > nfds ? xenconsoled_fd + 1 : nfds;
}
- if (bootloader_prod == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE
&& bootloader_cons == 0)) {
+ if (bootloader_prod < BOOTLOADER_BUF_SIZE) {
FD_SET(bootloader_fd, &rsel);
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
FD_ZERO(&wsel);
- if (bootloader_prod != bootloader_cons) {
+ if (bootloader_prod > 0) {
FD_SET(xenconsoled_fd, &wsel);
nfds = xenconsoled_fd + 1 > nfds ? xenconsoled_fd + 1 : nfds;
}
- if (xenconsoled_prod != xenconsoled_cons) {
+ if (xenconsoled_prod > 0) {
FD_SET(bootloader_fd, &wsel);
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
- ret = select(nfds, &rsel, &wsel, NULL, NULL);
+ ret = select(nfds, &rsel, &wsel, NULL, &wait);
if (ret < 0)
goto out_err;
+ timeout = ret == 0 ? 1 : 0;
/* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
- if (FD_ISSET(xenconsoled_fd, &rsel)) {
+ if (FD_ISSET(xenconsoled_fd, &rsel) || timeout) {
+ if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
+ if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ if (read_ahead >= XENCONSOLED_BUF_SIZE) /* The whole buffer
will be overwritten */
+ read_ahead = XENCONSOLED_BUF_SIZE;
+ else
+ memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead],
XENCONSOLED_BUF_SIZE - read_ahead);
+ xenconsoled_prod -= read_ahead;
+ }
ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod],
XENCONSOLED_BUF_SIZE - xenconsoled_prod);
if (ret < 0 && errno != EIO && errno != EAGAIN)
goto out_err;
@@ -229,7 +257,16 @@ static char * bootloader_interact(libxl_
}
/* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
- if (FD_ISSET(bootloader_fd, &rsel)) {
+ if (FD_ISSET(bootloader_fd, &rsel) || timeout) {
+ if (bootloader_prod == BOOTLOADER_BUF_SIZE) {
+ if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ if (read_ahead >= BOOTLOADER_BUF_SIZE) /* The whole buffer
will be overwritten */
+ read_ahead = BOOTLOADER_BUF_SIZE;
+ else
+ memmove(bootloader_buf, &bootloader_buf[read_ahead],
BOOTLOADER_BUF_SIZE - read_ahead);
+ bootloader_prod -= read_ahead;
+ }
ret = read(bootloader_fd, &bootloader_buf[bootloader_prod],
BOOTLOADER_BUF_SIZE - bootloader_prod);
if (ret < 0 && errno != EIO && errno != EAGAIN)
goto out_err;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Oct-17 16:17 UTC
Re: [Xen-devel] [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
Roger Pau Monne writes ("[Xen-devel] [PATCH v2] libxl: reimplement buffer
for bootloading and drop data if buffer is full"):> + /* Move buffers around to drop already consumed data */
> + if (xenconsoled_cons > 0) {
> + xenconsoled_prod -= xenconsoled_cons;
> + memmove(xenconsoled_buf,
&xenconsoled_buf[xenconsoled_cons], xenconsoled_prod);
This has quite a few overly-long lines. Can you keep them to 75ish
please ?
> - ret = select(nfds, &rsel, &wsel, NULL, NULL);
> + ret = select(nfds, &rsel, &wsel, NULL, &wait);
> if (ret < 0)
> goto out_err;
This needs to handle EINTR. Ie, if EINTR happens, just loop again.
(A better implementation would actually subtract the timeout but
let''s not do that now).
> + timeout = ret == 0 ? 1 : 0;
This seems a slightly odd approach. How about
if (ret==0) {
empty the ring buffer
continue;
}
?
And you should avoid setting a timeout if the buffer is empty, so that
a completely idle setup just sits and waits rather than polling every
second.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Roger Pau Monné
2011-Oct-18 10:35 UTC
Re: [Xen-devel] [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011/10/17 Ian Jackson <Ian.Jackson@eu.citrix.com>:> Roger Pau Monne writes ("[Xen-devel] [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full"): >> + /* Move buffers around to drop already consumed data */ >> + if (xenconsoled_cons > 0) { >> + xenconsoled_prod -= xenconsoled_cons; >> + memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons], xenconsoled_prod); > > This has quite a few overly-long lines. Can you keep them to 75ish > please ? > >> - ret = select(nfds, &rsel, &wsel, NULL, NULL); >> + ret = select(nfds, &rsel, &wsel, NULL, &wait); >> if (ret < 0) >> goto out_err; > > This needs to handle EINTR. Ie, if EINTR happens, just loop again. > (A better implementation would actually subtract the timeout but > let''s not do that now). > >> + timeout = ret == 0 ? 1 : 0; > > This seems a slightly odd approach. How about > if (ret==0) { > empty the ring bufferWhen talking with Ian Campbell we decided to avoid cleaning the whole buffer, and instead append data at the end, to prevent loosing more output than necessary.> continue; > } > ? > > And you should avoid setting a timeout if the buffer is empty, so that > a completely idle setup just sits and waits rather than polling every > second.Yes, will fix that.> Ian. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel