Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2011-10-18 at 14:38 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> # Date 1318944898 -7200
> # Node ID f7b311b85973f5862c323fed4e89c20b7af139a8
> # Parent  6fd16bcdd3e55bf8cc6e6e90e910f32e37654fd7
> 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 the whole buffer 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 6fd16bcdd3e5 -r f7b311b85973 tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c  Tue Oct 18 15:10:04 2011 +0200
> +++ b/tools/libxl/libxl_bootloader.c  Tue Oct 18 15:34:58 2011 +0200
> @@ -28,7 +28,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,
> @@ -169,6 +170,7 @@ static char * bootloader_interact(libxl_
>  
>      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 +183,66 @@ 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) {

It's a little less obvious now that this test means "there is data to
consume", a comment might be appropriate.

>              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);
> -        if (ret < 0)
> +        if (xenconsoled_prod == XENCONSOLED_BUF_SIZE ||
> +            bootloader_prod == BOOTLOADER_BUF_SIZE)
> +            ret = select(nfds, &rsel, &wsel, NULL, &wait);
> +        else
> +            ret = select(nfds, &rsel, &wsel, NULL, NULL);
> +        if (ret < 0) {
> +            if (errno == EINTR)
> +                continue;
>              goto out_err;
> +        }
>  
>          /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
> -        if (FD_ISSET(xenconsoled_fd, &rsel)) {
> +        if (ret == 0 && xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
> +            /* Drop the buffer */
> +            xenconsoled_prod = 0;

Do you also need to reset cons here? I expect we could prove it was
always zero anyway but it might be more obvious to just reset it.

Otherwise this looks good, thanks.

> +        } else if (FD_ISSET(xenconsoled_fd, &rsel)) {
>              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 +258,10 @@ static char * bootloader_interact(libxl_
>          }
>  
>          /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
> -        if (FD_ISSET(bootloader_fd, &rsel)) {
> +        if (ret == 0 && bootloader_prod == BOOTLOADER_BUF_SIZE) {
> +            /* Drop the buffer */
> +            bootloader_prod = 0;
> +        } else if (FD_ISSET(bootloader_fd, &rsel)) {
>              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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel