[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial speci
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial specific code |
Date: |
Thu, 1 Nov 2012 14:15:32 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Thu, Nov 01, 2012 at 02:33:29PM -0200, Luiz Capitulino wrote:
> On Thu, 1 Nov 2012 11:22:13 -0500
> Michael Roth <address@hidden> wrote:
>
> > On Wed, Oct 31, 2012 at 03:45:19PM -0200, Luiz Capitulino wrote:
> > > This commit prepares ga_channel_new(), ga_channel_read() and
> > > ga_channel_open() for isa-serial support.
> > >
> > > Signed-off-by: Luiz Capitulino <address@hidden>
> >
> > Reviewed-by: Michael Roth <address@hidden>
> >
> > Though I'm wondering if it'd be nicer to:
> >
> > 1) Add a GAChannelOps struct with pointers for init/read/write/write_all
> > 2) Move all the method-specific init stuff from ga_channel_new() into
> > method-specific init functions (which all use ga_channel_open() as
> > a general helper function, or maybe roll the CreateFile() into the
> > init routines with the proper flags hard-coded
> > 3) Have ga_channel_new() simply hang the proper GAChannelOps* off of
> > the GAChannel it create and then call GAChannel->ops->init(self)
> > 4) Make ga_channel_read/write/write_all(self) wrappers around
> > GAChannel->ops->read/write/write_all(self)
>
> Can this be a future improvement? I was also wondering if it would be good
> to have GAOps instead of having global functions for win32 & posix.
Sure, I wouldn't hold up the series for it. I agree it seems like a good
future improvement.
>
> >
> > > ---
> > > qga/channel-win32.c | 75
> > > ++++++++++++++++++++++++++++++-----------------------
> > > 1 file changed, 42 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> > > index 8e259c3..c173270 100644
> > > --- a/qga/channel-win32.c
> > > +++ b/qga/channel-win32.c
> > > @@ -213,14 +213,20 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf,
> > > size_t size, gsize *count)
> > > return G_IO_STATUS_ERROR;
> > > }
> > >
> > > - *count = to_read = MIN(size, rs->pending);
> > > - if (to_read) {
> > > - memcpy(buf, rs->buf + rs->cur, to_read);
> > > - rs->cur += to_read;
> > > - rs->pending -= to_read;
> > > - status = G_IO_STATUS_NORMAL;
> > > - } else {
> > > - status = G_IO_STATUS_AGAIN;
> > > + switch (c->method) {
> > > + case GA_CHANNEL_VIRTIO_SERIAL:
> > > + *count = to_read = MIN(size, rs->pending);
> > > + if (to_read) {
> > > + memcpy(buf, rs->buf + rs->cur, to_read);
> > > + rs->cur += to_read;
> > > + rs->pending -= to_read;
> > > + status = G_IO_STATUS_NORMAL;
> > > + } else {
> > > + status = G_IO_STATUS_AGAIN;
> > > + }
> > > + break;
> > > + default:
> > > + abort(); /* impossible */
> > > }
> > >
> > > return status;
> > > @@ -285,23 +291,11 @@ GIOStatus ga_channel_write_all(GAChannel *c, const
> > > char *buf, size_t size)
> > > return status;
> > > }
> > >
> > > -static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
> > > - const gchar *path)
> > > +static gboolean ga_channel_open(GAChannel *c, const gchar *path, int
> > > flags)
> > > {
> > > - if (!method == GA_CHANNEL_VIRTIO_SERIAL) {
> > > - g_critical("unsupported communication method");
> > > - return false;
> > > - }
> > > -
> > > c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL,
> > > - OPEN_EXISTING,
> > > - FILE_FLAG_NO_BUFFERING |
> > > FILE_FLAG_OVERLAPPED, NULL);
> > > - if (c->handle == INVALID_HANDLE_VALUE) {
> > > - g_critical("error opening path");
> > > - return false;
> > > - }
> > > -
> > > - return true;
> > > + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING |
> > > flags, NULL);
> > > + return (c->handle == INVALID_HANDLE_VALUE) ? false : true;
> > > }
> > >
> > > GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
> > > @@ -310,27 +304,42 @@ GAChannel *ga_channel_new(GAChannelMethod method,
> > > const gchar *path,
> > > GAChannel *c = g_malloc0(sizeof(GAChannel));
> > > SECURITY_ATTRIBUTES sec_attrs;
> > >
> > > - if (!ga_channel_open(c, method, path)) {
> > > - g_critical("error opening channel");
> > > - g_free(c);
> > > - return NULL;
> > > + switch (method) {
> > > + case GA_CHANNEL_VIRTIO_SERIAL:
> > > + if (!ga_channel_open(c, path, FILE_FLAG_OVERLAPPED)) {
> > > + g_critical("error opening channel (path: %s)", path);
> > > + goto out_err;
> > > + }
> > > +
> > > + sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES);
> > > + sec_attrs.lpSecurityDescriptor = NULL;
> > > + sec_attrs.bInheritHandle = false;
> > > + c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE,
> > > NULL);
> > > + if (!c->rstate.ov.hEvent) {
> > > + g_critical("can't create event");
> > > + goto out_err;
> > > + }
> > > +
> > > + c->source = ga_channel_create_watch_ov(c);
> > > + break;
> > > + default:
> > > + g_critical("unsupported communication method");
> > > + goto out_err;
> > > }
> > >
> > > c->cb = cb;
> > > c->user_data = opaque;
> > > c->method = method;
> > >
> > > - sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES);
> > > - sec_attrs.lpSecurityDescriptor = NULL;
> > > - sec_attrs.bInheritHandle = false;
> > > -
> > > c->rstate.buf_size = QGA_READ_COUNT_DEFAULT;
> > > c->rstate.buf = g_malloc(QGA_READ_COUNT_DEFAULT);
> > > - c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL);
> > >
> > > - c->source = ga_channel_create_watch_ov(c);
> > > g_source_attach(c->source, NULL);
> > > return c;
> > > +
> > > +out_err:
> > > + g_free(c);
> > > + return NULL;
> > > }
> > >
> > > void ga_channel_free(GAChannel *c)
> > > --
> > > 1.7.12.315.g682ce8b
> > >
> >
>