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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
> >> http://lists.xensource.com/xen-devel
> >
> >
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel