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
|
Home