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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 1 Nov 2012 14:13:52 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> 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.

That's correct.

> 
> > 
> > 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.

I meant to do that only for isa-serial, but what you suggest below is better.

> 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.

I can't tell you how much time I spent looking for a function that would
give me that info! This is really the right of of implementing this.

>  - 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
> > > > 
> > > > 
> > > 
> > 
> > 
>