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

[Roger Pau Monné <roger.pau@xxxxxxxxxxxxx>]

2011/10/17 Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v2] libxl: reimplement buffer for 
> bootloading and drop data if buffer is full"):
>> +        /* 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);
>
> This has quite a few overly-long lines.  Can you keep them to 75ish
> please ?
>
>> -        ret = select(nfds, &rsel, &wsel, NULL, NULL);
>> +        ret = select(nfds, &rsel, &wsel, NULL, &wait);
>>          if (ret < 0)
>>              goto out_err;
>
> This needs to handle EINTR.  Ie, if EINTR happens, just loop again.
> (A better implementation would actually subtract the timeout but
> let's not do that now).
>
>> +        timeout = ret == 0 ? 1 : 0;
>
> This seems a slightly odd approach.  How about
>  if (ret==0) {
>     empty the ring buffer

When talking with Ian Campbell we decided to avoid cleaning the whole
buffer, and instead append data at the end, to prevent loosing more
output than necessary.

>     continue;
>  }
> ?
>
> And you should avoid setting a timeout if the buffer is empty, so that
> a completely idle setup just sits and waits rather than polling every
> second.

Yes, will fix that.

> Ian.
>

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