Roger Pau Monne
2011-Oct-18 13:38 UTC
[Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# 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@entel.upc.edu>
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) {
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;
+ } 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@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-18 13:58 UTC
Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full
On Tue, 2011-10-18 at 14:38 +0100, Roger Pau Monne wrote:> # HG changeset patch > # User Roger Pau Monne <roger.pau@entel.upc.edu> > # 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@entel.upc.edu> > > 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@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Roger Pau Monné
2011-Oct-19 07:42 UTC
Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011/10/18 Ian Campbell <Ian.Campbell@citrix.com>:> On Tue, 2011-10-18 at 14:38 +0100, Roger Pau Monne wrote: >> # HG changeset patch >> # User Roger Pau Monne <roger.pau@entel.upc.edu> >> # 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@entel.upc.edu> >> >> 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@lists.xensource.com >> http://lists.xensource.com/xen-devel > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-19 10:34 UTC
Re: [Xen-devel] [PATCH v4] libxl: reimplement buffer for bootloading and drop data if buffer is full
On Wed, 2011-10-19 at 08:42 +0100, Roger Pau Monné wrote:> >> 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.I think you are correct.> >> /* 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.So I guess it is more "documentation" than useful code. I think zeroing cons would be harmless and make the code more obvious.> > Otherwise this looks good, thanks. > > Thanks to you and Ian Jackson for reviewing the code and having so > much patience.No problem. Ian.> > >> + } 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@lists.xensource.com > >> http://lists.xensource.com/xen-devel > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel