Roger Pau Monne
2011-Oct-11 12:26 UTC
[Xen-devel] [PATCH] 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 1318335991 -7200
# Node ID 2fb4bf8c16cd35ddc0bf7ddc7ff8fda4b9678211
# Parent 64f17c7e6c33e5f1c22711ae9cbdcbe191c20062
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 output from bootloader 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 64f17c7e6c33 -r 2fb4bf8c16cd tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c Tue Oct 11 10:26:32 2011 +0200
+++ b/tools/libxl/libxl_bootloader.c Tue Oct 11 14:26:31 2011 +0200
@@ -21,6 +21,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <sys/ioctl.h>
#include "libxl.h"
#include "libxl_internal.h"
@@ -28,7 +29,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,
@@ -165,10 +167,11 @@ static pid_t fork_exec_bootloader(int *m
*/
static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int
bootloader_fd, int fifo_fd)
{
- int ret;
+ int ret, read_ahead, timeout = 0;
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;
@@ -177,6 +180,10 @@ static char * bootloader_interact(libxl_
int bootloader_prod = 0, bootloader_cons = 0;
char bootloader_buf[BOOTLOADER_BUF_SIZE];
+ /* Set timeout to 1s before starting to discard data */
+ wait.tv_sec = BOOTLOADER_TIMEOUT;
+ wait.tv_usec = 0;
+
while(1) {
fd_set wsel, rsel;
int nfds;
@@ -185,15 +192,26 @@ static char * bootloader_interact(libxl_
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_prod > 0) {
+ xenconsoled_prod -= xenconsoled_cons;
+ memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons],
xenconsoled_prod);
+ xenconsoled_cons = 0;
+ }
+ if (bootloader_prod > 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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE
&& xenconsoled_cons == 0) || timeout) {
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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE
&& bootloader_cons == 0) || timeout) {
FD_SET(bootloader_fd, &rsel);
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
@@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
- ret = select(nfds, &rsel, &wsel, NULL, NULL);
+ ret = select(nfds, &rsel, &wsel, NULL, &wait);
if (ret < 0)
goto out_err;
+ if (ret == 0) {
+ timeout = 1;
+ continue;
+ }
+ timeout = 0;
/* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
if (FD_ISSET(xenconsoled_fd, &rsel)) {
+ if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
+ if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead],
XENCONSOLED_BUF_SIZE - read_ahead);
+ xenconsoled_prod -= read_ahead;
+ }
ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod],
XENCONSOLED_BUF_SIZE - xenconsoled_prod);
if (ret < 0 && errno != EIO && errno != EAGAIN)
goto out_err;
@@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_
/* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
if (FD_ISSET(bootloader_fd, &rsel)) {
+ if (bootloader_prod == BOOTLOADER_BUF_SIZE) {
+ if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ memmove(bootloader_buf, &bootloader_buf[read_ahead],
BOOTLOADER_BUF_SIZE - read_ahead);
+ bootloader_prod -= read_ahead;
+ }
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
Roger Pau Monne
2011-Oct-11 12:47 UTC
[Xen-devel] [PATCH v2] 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 1318337123 -7200
# Node ID a0b8b4fb418af35cd2e2d34b11883f8250c40a35
# Parent 64f17c7e6c33e5f1c22711ae9cbdcbe191c20062
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 output from bootloader 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 64f17c7e6c33 -r a0b8b4fb418a tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c Tue Oct 11 10:26:32 2011 +0200
+++ b/tools/libxl/libxl_bootloader.c Tue Oct 11 14:45:23 2011 +0200
@@ -21,6 +21,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <sys/ioctl.h>
#include "libxl.h"
#include "libxl_internal.h"
@@ -28,7 +29,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,
@@ -165,10 +167,11 @@ static pid_t fork_exec_bootloader(int *m
*/
static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int
bootloader_fd, int fifo_fd)
{
- int ret;
+ int ret, read_ahead, timeout = 0;
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;
@@ -177,6 +180,10 @@ static char * bootloader_interact(libxl_
int bootloader_prod = 0, bootloader_cons = 0;
char bootloader_buf[BOOTLOADER_BUF_SIZE];
+ /* Set timeout to 1s before starting to discard data */
+ wait.tv_sec = BOOTLOADER_TIMEOUT;
+ wait.tv_usec = 0;
+
while(1) {
fd_set wsel, rsel;
int nfds;
@@ -185,15 +192,26 @@ static char * bootloader_interact(libxl_
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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE
&& xenconsoled_cons == 0) || timeout) {
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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE
&& bootloader_cons == 0) || timeout) {
FD_SET(bootloader_fd, &rsel);
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
@@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_
nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds;
}
- ret = select(nfds, &rsel, &wsel, NULL, NULL);
+ ret = select(nfds, &rsel, &wsel, NULL, &wait);
if (ret < 0)
goto out_err;
+ if (ret == 0) {
+ timeout = 1;
+ continue;
+ }
+ timeout = 0;
/* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */
if (FD_ISSET(xenconsoled_fd, &rsel)) {
+ if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) {
+ if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead],
XENCONSOLED_BUF_SIZE - read_ahead);
+ xenconsoled_prod -= read_ahead;
+ }
ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod],
XENCONSOLED_BUF_SIZE - xenconsoled_prod);
if (ret < 0 && errno != EIO && errno != EAGAIN)
goto out_err;
@@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_
/* Input from bootloader, read bootloader_fd, write xenconsoled_fd */
if (FD_ISSET(bootloader_fd, &rsel)) {
+ if (bootloader_prod == BOOTLOADER_BUF_SIZE) {
+ if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0)
+ goto out_err;
+ memmove(bootloader_buf, &bootloader_buf[read_ahead],
BOOTLOADER_BUF_SIZE - read_ahead);
+ bootloader_prod -= read_ahead;
+ }
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-12 09:11 UTC
[Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full
On Tue, 2011-10-11 at 13:26 +0100, Roger Pau Monne wrote:> # HG changeset patch > # User Roger Pau Monne <roger.pau@entel.upc.edu> > # Date 1318335991 -7200 > # Node ID 2fb4bf8c16cd35ddc0bf7ddc7ff8fda4b9678211 > # Parent 64f17c7e6c33e5f1c22711ae9cbdcbe191c20062 > 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 output from bootloader 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 64f17c7e6c33 -r 2fb4bf8c16cd tools/libxl/libxl_bootloader.c > --- a/tools/libxl/libxl_bootloader.c Tue Oct 11 10:26:32 2011 +0200 > +++ b/tools/libxl/libxl_bootloader.c Tue Oct 11 14:26:31 2011 +0200 > @@ -21,6 +21,7 @@ > > #include <sys/stat.h> > #include <sys/types.h> > +#include <sys/ioctl.h> > > #include "libxl.h" > #include "libxl_internal.h" > @@ -28,7 +29,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, > @@ -165,10 +167,11 @@ static pid_t fork_exec_bootloader(int *m > */ > static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int bootloader_fd, int fifo_fd) > { > - int ret; > + int ret, read_ahead, timeout = 0; > > 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; > @@ -177,6 +180,10 @@ static char * bootloader_interact(libxl_ > int bootloader_prod = 0, bootloader_cons = 0; > char bootloader_buf[BOOTLOADER_BUF_SIZE]; > > + /* Set timeout to 1s before starting to discard data */ > + wait.tv_sec = BOOTLOADER_TIMEOUT; > + wait.tv_usec = 0;There are some portability snaggles with the timeout argument to a select (IIRC Linux can modify it). I think it would be wise to reinitialise this right before each select call.> + > while(1) { > fd_set wsel, rsel; > int nfds; > @@ -185,15 +192,26 @@ static char * bootloader_interact(libxl_ > 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_prod > 0) { > + xenconsoled_prod -= xenconsoled_cons; > + memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons], xenconsoled_prod); > + xenconsoled_cons = 0; > + } > + if (bootloader_prod > 0) { > + bootloader_prod -= bootloader_cons; > + memmove(bootloader_buf, &bootloader_buf[bootloader_cons], bootloader_prod); > + bootloader_cons = 0; > + }If the timeout occurs then we will drop through and each of the FD_ISSET will fail and we will loop back round to the top and do this processing before trying again, which will ensure that xenconsoled_cons == 0. I think this removes the need for the timeout var you added, as well as the associated continue statement and (I think) makes the control flow simpler.> 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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) {If you''ve had a timeout there will be data pending on this FD won''t there, so this change essentially means that after the first timeout the timeout on the select becomes effectively zero? Can you just make this: if (xenconsoled_prod == 0 || xenconsoled_cons == 0) { The previous memmove will ensure we hit this case. Then inside the if FD_ISSET(xenconsoled_fd) you can check for prod == BOOTLOADER_BUF_SIZE and discard from the buffer (e.g. we can discard the tail just by modifying prod?). Hmm. that might discard a bit too aggressively, perhaps leaving this check as it was and doing the discard if (ret == 0 && xenconsoled_prod == BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works?> 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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) { > FD_SET(bootloader_fd, &rsel); > nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; > } > @@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_ > nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; > } > > - ret = select(nfds, &rsel, &wsel, NULL, NULL); > + ret = select(nfds, &rsel, &wsel, NULL, &wait); > if (ret < 0) > goto out_err; > + if (ret == 0) { > + timeout = 1; > + continue; > + } > + timeout = 0; > > /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */ > if (FD_ISSET(xenconsoled_fd, &rsel)) { > + if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) { > + if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0)I think we can avoid this as described above, but if not then you need to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE.> + goto out_err; > + memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead], XENCONSOLED_BUF_SIZE - read_ahead); > + xenconsoled_prod -= read_ahead; > + } > ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - xenconsoled_prod); > if (ret < 0 && errno != EIO && errno != EAGAIN) > goto out_err; > @@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_ > > /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */ > if (FD_ISSET(bootloader_fd, &rsel)) { > + if (bootloader_prod == BOOTLOADER_BUF_SIZE) { > + if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0) > + goto out_err; > + memmove(bootloader_buf, &bootloader_buf[read_ahead], BOOTLOADER_BUF_SIZE - read_ahead); > + bootloader_prod -= read_ahead; > + } > 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
Roger Pau Monné
2011-Oct-13 08:43 UTC
[Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011/10/12 Ian Campbell <Ian.Campbell@citrix.com>:> On Tue, 2011-10-11 at 13:26 +0100, Roger Pau Monne wrote: >> # HG changeset patch >> # User Roger Pau Monne <roger.pau@entel.upc.edu> >> # Date 1318335991 -7200 >> # Node ID 2fb4bf8c16cd35ddc0bf7ddc7ff8fda4b9678211 >> # Parent 64f17c7e6c33e5f1c22711ae9cbdcbe191c20062 >> 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 output from bootloader 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 64f17c7e6c33 -r 2fb4bf8c16cd tools/libxl/libxl_bootloader.c >> --- a/tools/libxl/libxl_bootloader.c Tue Oct 11 10:26:32 2011 +0200 >> +++ b/tools/libxl/libxl_bootloader.c Tue Oct 11 14:26:31 2011 +0200 >> @@ -21,6 +21,7 @@ >> >> #include <sys/stat.h> >> #include <sys/types.h> >> +#include <sys/ioctl.h> >> >> #include "libxl.h" >> #include "libxl_internal.h" >> @@ -28,7 +29,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, >> @@ -165,10 +167,11 @@ static pid_t fork_exec_bootloader(int *m >> */ >> static char * bootloader_interact(libxl__gc *gc, int xenconsoled_fd, int bootloader_fd, int fifo_fd) >> { >> - int ret; >> + int ret, read_ahead, timeout = 0; >> >> 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; >> @@ -177,6 +180,10 @@ static char * bootloader_interact(libxl_ >> int bootloader_prod = 0, bootloader_cons = 0; >> char bootloader_buf[BOOTLOADER_BUF_SIZE]; >> >> + /* Set timeout to 1s before starting to discard data */ >> + wait.tv_sec = BOOTLOADER_TIMEOUT; >> + wait.tv_usec = 0; > > There are some portability snaggles with the timeout argument to a > select (IIRC Linux can modify it). I think it would be wise to > reinitialise this right before each select call.Yes, I didn''t remember Linux can modify the timeout struct when returning from select, fixed it.>> + >> while(1) { >> fd_set wsel, rsel; >> int nfds; >> @@ -185,15 +192,26 @@ static char * bootloader_interact(libxl_ >> 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_prod > 0) { >> + xenconsoled_prod -= xenconsoled_cons; >> + memmove(xenconsoled_buf, &xenconsoled_buf[xenconsoled_cons], xenconsoled_prod); >> + xenconsoled_cons = 0; >> + } >> + if (bootloader_prod > 0) { >> + bootloader_prod -= bootloader_cons; >> + memmove(bootloader_buf, &bootloader_buf[bootloader_cons], bootloader_prod); >> + bootloader_cons = 0; >> + } > > If the timeout occurs then we will drop through and each of the FD_ISSET > will fail and we will loop back round to the top and do this processing > before trying again, which will ensure that xenconsoled_cons == 0. > > I think this removes the need for the timeout var you added, as well as > the associated continue statement and (I think) makes the control flow > simpler. > >> 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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) { > > If you''ve had a timeout there will be data pending on this FD won''t > there, so this change essentially means that after the first timeout the > timeout on the select becomes effectively zero?Well, I cannot know this for sure, but it''s the most likely situation.> Can you just make this: > if (xenconsoled_prod == 0 || xenconsoled_cons == 0) {I don''t think there''s any need for this pair of "ifs", since they will always be hit, because xenconsoled_cons and bootloader_cons is always 0 at this point, I agree with you that this might be dropping data to aggressively, since the timeout will not be needed. Something like this might be more suitable: if (bootloader_prod == 0 || ret == 0) { The "timeout" variable is not really needed, I''ve just added it to make the code cleaner and easier to understand, but ret can be used in the same way. From my point of view, we should only add the file descriptors with a full buffer to the select set after a timeout has occurred, since checking for a timeout (ret == 0) in the FD_ISSET may make us enter an infinite loop (because there''s data pending on the file descriptor, and the timeout is not hit).> > The previous memmove will ensure we hit this case. Then inside the if > FD_ISSET(xenconsoled_fd) you can check for prod == BOOTLOADER_BUF_SIZE > and discard from the buffer (e.g. we can discard the tail just by > modifying prod?).We are discarding from the tail? I''m not sure it''s the best way to do it, pygrub uses curses control sequences, and I think it''s best to try to keep the stream as clean as possible, dropping the tail would really mess up with screen printing (maybe just mess up a frame, but the output will be rubbish).> Hmm. that might discard a bit too aggressively, perhaps leaving this > check as it was and doing the discard if (ret == 0 && xenconsoled_prod > == BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works?This might make the select loop too fast, since we add all the descriptors to the set, and it''s a strong possibility that the timeout will never be hit, so ret will never be 0 and we will be looping forever.>> 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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) { >> FD_SET(bootloader_fd, &rsel); >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; >> } >> @@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_ >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; >> } >> >> - ret = select(nfds, &rsel, &wsel, NULL, NULL); >> + ret = select(nfds, &rsel, &wsel, NULL, &wait); >> if (ret < 0) >> goto out_err; >> + if (ret == 0) { >> + timeout = 1; >> + continue; >> + } >> + timeout = 0; >> >> /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */ >> if (FD_ISSET(xenconsoled_fd, &rsel)) { >> + if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) { >> + if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0) > > I think we can avoid this as described above, but if not then you need > to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE.Yes, I''ve also missed that one, this should be checked to avoid passing a negative integer to read or moving memory regions outside of the buffer.>> + goto out_err; >> + memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead], XENCONSOLED_BUF_SIZE - read_ahead); >> + xenconsoled_prod -= read_ahead; >> + } >> ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - xenconsoled_prod); >> if (ret < 0 && errno != EIO && errno != EAGAIN) >> goto out_err; >> @@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_ >> >> /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */ >> if (FD_ISSET(bootloader_fd, &rsel)) { >> + if (bootloader_prod == BOOTLOADER_BUF_SIZE) { >> + if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0) >> + goto out_err; >> + memmove(bootloader_buf, &bootloader_buf[read_ahead], BOOTLOADER_BUF_SIZE - read_ahead); >> + bootloader_prod -= read_ahead; >> + } >> ret = read(bootloader_fd, &bootloader_buf[bootloader_prod], BOOTLOADER_BUF_SIZE - bootloader_prod); >> if (ret < 0 && errno != EIO && errno != EAGAIN) >> goto out_err; > > >I''m wrapping another patch which I will send right now. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-13 09:06 UTC
[Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full
On Thu, 2011-10-13 at 09:43 +0100, Roger Pau Monné wrote:> >> 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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) { > > > > If you''ve had a timeout there will be data pending on this FD won''t > > there, so this change essentially means that after the first timeout the > > timeout on the select becomes effectively zero? > > Well, I cannot know this for sure, but it''s the most likely situation. > > > Can you just make this: > > if (xenconsoled_prod == 0 || xenconsoled_cons == 0) { > > I don''t think there''s any need for this pair of "ifs", since they will > always be hit, because xenconsoled_cons and bootloader_cons is always > 0 at this point, I agree with you that this might be dropping data to > aggressively, since the timeout will not be needed. Something like > this might be more suitable: > > if (bootloader_prod == 0 || ret == 0) { > > The "timeout" variable is not really needed, I''ve just added it to > make the code cleaner and easier to understand,Actually, the timeout variable was ok my concern was more the use of continue and the code flow it seemed to create which wasn''t easy to follow.> but ret can be used in > the same way. From my point of view, we should only add the file > descriptors with a full buffer to the select set after a timeout has > occurred, since checking for a timeout (ret == 0) in the FD_ISSET may > make us enter an infinite loop (because there''s data pending on the > file descriptor, and the timeout is not hit).If the buffer is full and we therefore don''t add it to the select then either * some other file descriptor in the select becomes read for read/write as appropriate, in which case some progress will be made, - or - * the timeout will fire and we will discard some data, such that next time round the loop we can make progress. Do we need to care about the case where no fd''s get added to the select?> > The previous memmove will ensure we hit this case. Then inside the if > > FD_ISSET(xenconsoled_fd) you can check for prod == BOOTLOADER_BUF_SIZE > > and discard from the buffer (e.g. we can discard the tail just by > > modifying prod?). > > We are discarding from the tail? I''m not sure it''s the best way to do > it, pygrub uses curses control sequences, and I think it''s best to try > to keep the stream as clean as possible, dropping the tail would > really mess up with screen printing (maybe just mess up a frame, but > the output will be rubbish).Doesn''t dropping any part of the data stream have that same property? Is dropping the tail somehow worse?> > > Hmm. that might discard a bit too aggressively, perhaps leaving this > > check as it was and doing the discard if (ret == 0 && xenconsoled_prod > > == BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works? > > This might make the select loop too fast, since we add all the > descriptors to the set, and it''s a strong possibility that the timeout > will never be hit, so ret will never be 0 and we will be looping > forever.That''s why we should not add fd''s to the set which we cannot make progress with if they become/are ready. Ian.> > >> 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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) { > >> FD_SET(bootloader_fd, &rsel); > >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; > >> } > >> @@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_ > >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; > >> } > >> > >> - ret = select(nfds, &rsel, &wsel, NULL, NULL); > >> + ret = select(nfds, &rsel, &wsel, NULL, &wait); > >> if (ret < 0) > >> goto out_err; > >> + if (ret == 0) { > >> + timeout = 1; > >> + continue; > >> + } > >> + timeout = 0; > >> > >> /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */ > >> if (FD_ISSET(xenconsoled_fd, &rsel)) { > >> + if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) { > >> + if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0) > > > > I think we can avoid this as described above, but if not then you need > > to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE. > > Yes, I''ve also missed that one, this should be checked to avoid > passing a negative integer to read or moving memory regions outside of > the buffer. > > >> + goto out_err; > >> + memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead], XENCONSOLED_BUF_SIZE - read_ahead); > >> + xenconsoled_prod -= read_ahead; > >> + } > >> ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - xenconsoled_prod); > >> if (ret < 0 && errno != EIO && errno != EAGAIN) > >> goto out_err; > >> @@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_ > >> > >> /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */ > >> if (FD_ISSET(bootloader_fd, &rsel)) { > >> + if (bootloader_prod == BOOTLOADER_BUF_SIZE) { > >> + if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0) > >> + goto out_err; > >> + memmove(bootloader_buf, &bootloader_buf[read_ahead], BOOTLOADER_BUF_SIZE - read_ahead); > >> + bootloader_prod -= read_ahead; > >> + } > >> ret = read(bootloader_fd, &bootloader_buf[bootloader_prod], BOOTLOADER_BUF_SIZE - bootloader_prod); > >> if (ret < 0 && errno != EIO && errno != EAGAIN) > >> goto out_err; > > > > > > > > I''m wrapping another patch which I will send right now._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Roger Pau Monné
2011-Oct-13 09:32 UTC
[Xen-devel] Re: [PATCH] libxl: reimplement buffer for bootloading and drop data if buffer is full
2011/10/13 Ian Campbell <Ian.Campbell@citrix.com>:> On Thu, 2011-10-13 at 09:43 +0100, Roger Pau Monné wrote: > >> >> 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 == 0 || (xenconsoled_prod < BOOTLOADER_BUF_SIZE && xenconsoled_cons == 0) || timeout) { >> > >> > If you''ve had a timeout there will be data pending on this FD won''t >> > there, so this change essentially means that after the first timeout the >> > timeout on the select becomes effectively zero? >> >> Well, I cannot know this for sure, but it''s the most likely situation. >> >> > Can you just make this: >> > if (xenconsoled_prod == 0 || xenconsoled_cons == 0) { >> >> I don''t think there''s any need for this pair of "ifs", since they will >> always be hit, because xenconsoled_cons and bootloader_cons is always >> 0 at this point, I agree with you that this might be dropping data to >> aggressively, since the timeout will not be needed. Something like >> this might be more suitable: >> >> if (bootloader_prod == 0 || ret == 0) { >> >> The "timeout" variable is not really needed, I''ve just added it to >> make the code cleaner and easier to understand, > > Actually, the timeout variable was ok my concern was more the use of > continue and the code flow it seemed to create which wasn''t easy to > follow.Ok, I think I''ve managed to write it in a more friendly way.> If the buffer is full and we therefore don''t add it to the select then > either > * some other file descriptor in the select becomes read for > read/write as appropriate, in which case some progress will be > made, - or - > * the timeout will fire and we will discard some data, such that > next time round the loop we can make progress. > > Do we need to care about the case where no fd''s get added to the select?I don''t think it''s possible, since read fd''s get added when there''s space in the buffer, and write fd''s gets added when there''s data in the buffer, so either way there will be fd''s in select. buffer empty: read fd''s are added buffer contains data but it''s not full: read & write fd''s are added buffer full: write fd''s are added.> Doesn''t dropping any part of the data stream have that same property? Is > dropping the tail somehow worse?Dropping from the head and adding data to the tail makes the data stream in the buffer contiguous, there are no breaks inside it. On the other hand, rewriting data in the tail (or the head) introduces one (or possible more) breaks in the data stream. What I mean with "break" is that the data is no longer in the same order as it came from the fd.>> >> > Hmm. that might discard a bit too aggressively, perhaps leaving this >> > check as it was and doing the discard if (ret == 0 && xenconsoled_prod >> > == BOOTLOADER_BUF_SIZE) in the else clause if the FD_ISSET works? >> >> This might make the select loop too fast, since we add all the >> descriptors to the set, and it''s a strong possibility that the timeout >> will never be hit, so ret will never be 0 and we will be looping >> forever. > > That''s why we should not add fd''s to the set which we cannot make > progress with if they become/are ready. > > Ian. > >> >> >> 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 == 0 || (bootloader_prod < BOOTLOADER_BUF_SIZE && bootloader_cons == 0) || timeout) { >> >> FD_SET(bootloader_fd, &rsel); >> >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; >> >> } >> >> @@ -208,12 +226,23 @@ static char * bootloader_interact(libxl_ >> >> nfds = bootloader_fd + 1 > nfds ? bootloader_fd + 1 : nfds; >> >> } >> >> >> >> - ret = select(nfds, &rsel, &wsel, NULL, NULL); >> >> + ret = select(nfds, &rsel, &wsel, NULL, &wait); >> >> if (ret < 0) >> >> goto out_err; >> >> + if (ret == 0) { >> >> + timeout = 1; >> >> + continue; >> >> + } >> >> + timeout = 0; >> >> >> >> /* Input from xenconsole, read xenconsoled_fd, write bootloader_fd */ >> >> if (FD_ISSET(xenconsoled_fd, &rsel)) { >> >> + if (xenconsoled_prod == XENCONSOLED_BUF_SIZE) { >> >> + if (ioctl(xenconsoled_fd, FIONREAD, &read_ahead) < 0) >> > >> > I think we can avoid this as described above, but if not then you need >> > to be wary of the case where read_ahead > XENCONSOLED_BUF_SIZE. >> >> Yes, I''ve also missed that one, this should be checked to avoid >> passing a negative integer to read or moving memory regions outside of >> the buffer. >> >> >> + goto out_err; >> >> + memmove(xenconsoled_buf, &xenconsoled_buf[read_ahead], XENCONSOLED_BUF_SIZE - read_ahead); >> >> + xenconsoled_prod -= read_ahead; >> >> + } >> >> ret = read(xenconsoled_fd, &xenconsoled_buf[xenconsoled_prod], XENCONSOLED_BUF_SIZE - xenconsoled_prod); >> >> if (ret < 0 && errno != EIO && errno != EAGAIN) >> >> goto out_err; >> >> @@ -230,6 +259,12 @@ static char * bootloader_interact(libxl_ >> >> >> >> /* Input from bootloader, read bootloader_fd, write xenconsoled_fd */ >> >> if (FD_ISSET(bootloader_fd, &rsel)) { >> >> + if (bootloader_prod == BOOTLOADER_BUF_SIZE) { >> >> + if (ioctl(bootloader_fd, FIONREAD, &read_ahead) < 0) >> >> + goto out_err; >> >> + memmove(bootloader_buf, &bootloader_buf[read_ahead], BOOTLOADER_BUF_SIZE - read_ahead); >> >> + bootloader_prod -= read_ahead; >> >> + } >> >> ret = read(bootloader_fd, &bootloader_buf[bootloader_prod], BOOTLOADER_BUF_SIZE - bootloader_prod); >> >> if (ret < 0 && errno != EIO && errno != EAGAIN) >> >> goto out_err; >> > >> > >> > >> >> I''m wrapping another patch which I will send right now. > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel