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

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2011/10/18 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;

Now that I also look at it, I think this "ifs" could be removed, the
following checks for cons > 0 will perform the same task if cons ==
prod, and the code will look cleaner.

>> + Â Â Â Â/* 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.

Added a couple of comments.

>> Â Â Â Â Â Â Â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.

It is always 0 because we check "cons > 0" at the start of the loop,
and set it to 0 if it is bigger. If you feel it's best to set it to
zero here to make the code easier to understand it's fine for me.

> Otherwise this looks good, thanks.

Thanks to you and Ian Jackson for reviewing the code and having so
much patience.

>> + Â Â Â Â} 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