|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full
On Tue, 2011-10-11 at 13:26 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> # Date 1318335991 -7200
> # Node ID 2fb4bf8c16cd35ddc0bf7ddc7ff8fda4b9678211
> # 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@xxxxxxxxxxxxx>
>
> diff -r 64f17c7e6c33 -r 2fb4bf8c16cd 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:26:31 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;
There are some portability snaggles with the timeout argument to a
select (IIRC Linux can modify it). I think it would be wise to
reinitialise this right before each select call.
> +
> 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_prod > 0) {
> + xenconsoled_prod -= xenconsoled_cons;
> + memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons],
> xenconsoled_prod);
> + xenconsoled_cons = 0;
> + }
> + if (bootloader_prod > 0) {
> + bootloader_prod -= bootloader_cons;
> + memmove(bootloader_buf, &bootloader_buf[bootloader_cons],
> bootloader_prod);
> + bootloader_cons = 0;
> + }
If the timeout occurs then we will drop through and each of the FD_ISSET
will fail and we will loop back round to the top and do this processing
before trying again, which will ensure that xenconsoled_cons == 0.
I think this removes the need for the timeout var you added, as well as
the associated continue statement and (I think) makes the control flow
simpler.
> 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) {
If you've had a timeout there will be data pending on this FD won't
there, so this change essentially means that after the first timeout the
timeout on the select becomes effectively zero?
Can you just make this:
if (xenconsoled_prod == 0 || xenconsoled_cons == 0) {
The previous memmove will ensure we hit this case. Then inside the if
FD_ISSET(xenconsoled_fd) you can check for prod == BOOTLOADER_BUF_SIZE
and discard from the buffer (e.g. we can discard the tail just by
modifying prod?).
Hmm. that might discard a bit too aggressively, perhaps leaving this
check as it was and doing the discard if (ret == 0 && xenconsoled_prod
== BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works?
> 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)
I think we can avoid this as described above, but if not then you need
to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE.
> + 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;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |