WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and

To: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Tue, 18 Oct 2011 14:58:08 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 18 Oct 2011 07:01:49 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <f7b311b85973f5862c32.1318945118@loki>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <f7b311b85973f5862c32.1318945118@loki>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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