Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Thu, Nov 01, 2012 at 02:55:20PM -0200, Luiz Capitulino wrote:
> On Thu, 1 Nov 2012 11:31:05 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Oct 31, 2012 at 03:45:20PM -0200, Luiz Capitulino wrote:
> > > It's implemented by opening the serial port in "non-blocking" mode
> > > and using the GSourceFuncs API in the following way:
> > > 
> > >  o prepare(): issue a read request. If something has been read, it's
> > >               stored in rs->buf and rs->pending will store the number
> > > 			  of bytes read. If there wasn't any bytes available, we
> > > 			  set the timeout to 500 ms and return
> > > 
> > >  o check():   returns true if prepare() was able to read anything,
> > >               otherwise return false
> > > 
> > >  o finalize(): does nothing
> > > 
> > > At dispatch() time we simply copy the bytes read to the buffer passed
> > > to ga_channel_read().
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  qga/channel-win32.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > > 
> > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> > > index c173270..887fe5f 100644
> > > --- a/qga/channel-win32.c
> > > +++ b/qga/channel-win32.c
> > > @@ -179,6 +179,46 @@ static gboolean ga_channel_dispatch_ov(GSource *source, GSourceFunc unused,
> > >      return success;
> > >  }
> > > 
> > > +static gboolean ga_channel_prepare(GSource *source, gint *timeout_ms)
> > > +{
> > > +    GAWatch *watch = (GAWatch *)source;
> > > +    GAChannel *c = (GAChannel *)watch->channel;
> > > +    GAChannelReadState *rs = &c->rstate;
> > > +    DWORD count_read = 0;
> > > +    bool success;
> > > +
> > > +    success = ReadFile(c->handle, rs->buf, rs->buf_size, &count_read, NULL);
> > > +    if (success) {
> > > +        if (count_read == 0) {
> > > +            *timeout_ms = 500;
> > > +            return false;
> > > +        }
> > > +        rs->pending = count_read;
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +static gboolean ga_channel_check(GSource *source)
> > > +{
> > > +    GAWatch *watch = (GAWatch *)source;
> > > +    GAChannel *c = (GAChannel *)watch->channel;
> > > +    GAChannelReadState *rs = &c->rstate;
> > > +
> > > +    return !!rs->pending;
> > > +}
> > > +
> > > +static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused,
> > > +                                    gpointer user_data)
> > > +{
> > > +    GAWatch *watch = (GAWatch *)source;
> > > +    GAChannel *c = (GAChannel *)watch->channel;
> > > +
> > > +    g_assert(c->cb);
> > > +    return c->cb(0, c->user_data);
> > > +}
> > > +
> > >  static void ga_channel_finalize_ov(GSource *source)
> > >  {
> > >      g_debug("finalize");
> > > @@ -203,6 +243,30 @@ static GSource *ga_channel_create_watch_ov(GAChannel *c)
> > >      return source;
> > >  }
> > > 
> > > +static void ga_channel_finalize(GSource *source)
> > > +{
> > > +    g_debug("finalize");
> > > +}
> > > +
> > > +GSourceFuncs ga_channel_watch_funcs = {
> > > +    ga_channel_prepare,
> > > +    ga_channel_check,
> > > +    ga_channel_dispatch,
> > > +    ga_channel_finalize
> > > +};
> > > +
> > > +static GSource *ga_channel_create_watch(GAChannel *c)
> > > +{
> > > +    GSource *source = g_source_new(&ga_channel_watch_funcs, sizeof(GAWatch));
> > > +    GAWatch *watch = (GAWatch *)source;
> > > +
> > > +    watch->channel = c;
> > > +    watch->pollfd.fd = -1;
> > > +    g_source_add_poll(source, &watch->pollfd);
> > > +
> > > +    return source;
> > > +}
> > > +
> > >  GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count)
> > >  {
> > >      GAChannelReadState *rs = &c->rstate;
> > > @@ -225,6 +289,12 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count)
> > >              status = G_IO_STATUS_AGAIN;
> > >          }
> > >          break;
> > > +    case GA_CHANNEL_ISA_SERIAL:
> > > +        memcpy(buf, rs->buf, rs->pending);
> > 
> > Do we really want to ignore size? Seems like we can overrun their buffer
> > if it's smaller than ours. You can use rs->start to track the number of
> > bytes theyve read so far and offset from that in prepare()
> 
> size == rs->buf_size, so we're ok. Although this is sensible, as both buffers
> are not tied...

Hmm, true. But there's no contract that users of ga_channel_read() have to
use size==QGA_READ_COUNT_DEFAULT, so this could cause problems if that
ever changed.

> 
> I think that the intermediate buffer handling complicate things, and for
> isa-serial this is not needed. What about returning the buffer from
> ga_channel_read()?
> 
> I mean, instead of:
> 
> gchar buf[QGA_READ_COUNT_DEFAULT+1];
> gsize count;
> ...
> 
> ga_channel_read(s->channel, buf, QGA_READ_COUNT_DEFAULT, &count);
> 
> We could have:
> 
> gchar *buf;
> gsize count;
> ...
> 
> ga_channel_read(s->channel, &buf, &count);

We don't expose our internal buffer for virtio-serial because if it
fills up, and there were partial reads (which might occur if size !=
QGA_READ_COUNT_DEFAULT) there may be unused space at the beginning of
the buffer, so we'll shift the used portion back to the beginning to
read as much as possible for each IO request. It's minor optimization,
but only requires a few lines of code to implement so I think it makes
sense. So that prevents us from passing the buffer directly to users
in that case.

If we want to avoid this for isa-serial, I'd avoiding the use of the
internal buffer completely. What you might be able to do instead is:

 - have your prepare() function call ClearCommError(), then check the returned
   lpStat information for the cbInQue value, which will give you the number of
   bytes available. If it's > 0, prepare() can set c->pending to this
   amount, and avoid the ReadFile() or using the internal buffer.

 - in ga_channel_read(), you can read MIN(size, c->pending) bytes directly
   into the buf it's called with via ReadFile().

> 
> > 
> > > +        *count = rs->pending;
> > > +        rs->pending = 0;
> > > +        status = G_IO_STATUS_NORMAL;
> > > +        break;
> > >      default:
> > >          abort(); /* impossible */
> > >      }
> > > @@ -303,6 +373,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
> > >  {
> > >      GAChannel *c = g_malloc0(sizeof(GAChannel));
> > >      SECURITY_ATTRIBUTES sec_attrs;
> > > +    COMMTIMEOUTS timeouts;
> > > 
> > >      switch (method) {
> > >      case GA_CHANNEL_VIRTIO_SERIAL:
> > > @@ -322,6 +393,22 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
> > > 
> > >          c->source = ga_channel_create_watch_ov(c);
> > >          break;
> > > +    case GA_CHANNEL_ISA_SERIAL:
> > > +        if (!ga_channel_open(c, path, 0)) {
> > > +            g_critical("error opening channel (path: %s)", path);
> > > +            goto out_err;
> > > +        }
> > > +
> > > +        /* non-blocking I/O */
> > > +        memset(&timeouts, 0, sizeof(timeouts));
> > > +        timeouts.ReadIntervalTimeout = MAXDWORD;
> > > +        if (!SetCommTimeouts(c->handle, &timeouts)) {
> > > +            CloseHandle(c->handle);
> > > +            goto out_err;
> > > +        }
> > > +
> > > +        c->source = ga_channel_create_watch(c);
> > > +        break;
> > >      default:
> > >          g_critical("unsupported communication method");
> > >          goto out_err;
> > > -- 
> > > 1.7.12.315.g682ce8b
> > > 
> > > 
> > 
> 
>