* [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011-10-11 12:26 [PATCH] " Roger Pau Monne
@ 2011-10-11 12:47 ` Roger Pau Monne
0 siblings, 0 replies; 4+ messages in thread
From: Roger Pau Monne @ 2011-10-11 12:47 UTC (permalink / raw
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1318337123 -7200
# Node ID a0b8b4fb418af35cd2e2d34b11883f8250c40a35
# 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 a0b8b4fb418a 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 Tue Oct 11 14:45: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 = 0;
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;
@@ -177,6 +180,10 @@ static char * bootloader_interact(libxl_
int bootloader_prod = 0, bootloader_cons = 0;
char bootloader_buf[BOOTLOADER_BUF_SIZE];
+ /* Set timeout to 1s before starting to discard data */
+ wait.tv_sec = BOOTLOADER_TIMEOUT;
+ wait.tv_usec = 0;
+
while(1) {
fd_set wsel, rsel;
int nfds;
@@ -185,15 +192,26 @@ static char * bootloader_interact(libxl_
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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) {
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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) {
FD_SET(bootloader_fd, &rsel);
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
@@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_
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;
+ if (ret == 0) {
+ timeout = 1;
+ continue;
+ }
+ timeout = 0;
/* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
if (FD_ISSET(xenconsoled_fd, &rsel)) {
+ if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
+ if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ 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;
@@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_
/* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
if (FD_ISSET(bootloader_fd, &rsel)) {
+ if (bootloader_prod == BOOTLOADER_BUF_SIZE) {
+ if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
@ 2011-10-13 11:04 Roger Pau Monne
2011-10-17 16:17 ` Ian Jackson
0 siblings, 1 reply; 4+ messages in thread
From: Roger Pau Monne @ 2011-10-13 11:04 UTC (permalink / raw
To: xen-devel; +Cc: Ian.Campbell
# 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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011-10-13 11:04 [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full Roger Pau Monne
@ 2011-10-17 16:17 ` Ian Jackson
2011-10-18 10:35 ` Roger Pau Monné
0 siblings, 1 reply; 4+ messages in thread
From: Ian Jackson @ 2011-10-17 16:17 UTC (permalink / raw
To: Roger Pau Monne; +Cc: xen-devel, Ian.Campbell
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011-10-17 16:17 ` Ian Jackson
@ 2011-10-18 10:35 ` Roger Pau Monné
0 siblings, 0 replies; 4+ messages in thread
From: Roger Pau Monné @ 2011-10-18 10:35 UTC (permalink / raw
To: Ian Jackson; +Cc: xen-devel, Ian.Campbell
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 buffer
When 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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-10-18 10:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-13 11:04 [PATCH v2] libxl: reimplement buffer for bootloading and drop data if buffer is full Roger Pau Monne
2011-10-17 16:17 ` Ian Jackson
2011-10-18 10:35 ` Roger Pau Monné
-- strict thread matches above, loose matches on Subject: below --
2011-10-11 12:26 [PATCH] " Roger Pau Monne
2011-10-11 12:47 ` [PATCH v2] " Roger Pau Monne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.