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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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