From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=32929 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PG9r3-00017I-G6 for qemu-devel@nongnu.org; Wed, 10 Nov 2010 07:32:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PG9r2-0002MU-1o for qemu-devel@nongnu.org; Wed, 10 Nov 2010 07:32:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44823) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PG9r1-0002MH-RD for qemu-devel@nongnu.org; Wed, 10 Nov 2010 07:32:44 -0500 Date: Wed, 10 Nov 2010 10:32:35 -0200 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 1/2] qemu-char: Introduce Buffered driver Message-ID: <20101110103235.1639f23a@doriath> In-Reply-To: References: <1288037204-27768-1-git-send-email-lcapitulino@redhat.com> <1288037204-27768-2-git-send-email-lcapitulino@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Wed, 10 Nov 2010 10:26:15 +0100 Markus Armbruster wrote: > Luiz Capitulino writes: > > > This driver handles in-memory qemu-char driver operations, that's, > > all writes to this driver are stored in an internal buffer. The > > driver doesn't talk to the external world in any way. > > I'm not so happy with the name "buffered driver". "Bufferedness" isn't > what this kind of character device is about. It's about being connected > to a memory buffer. > > Consider stdio streams or C++ IOstreams: there are various kinds of > streams, and they can be buffered or unbuffered. One kind is the > "memory" or "string" stream. Makes sense. > What about "memory driver"? "membuf driver"? "string driver"? Let's call it MemoryDriver then. > > Right now it's very simple: it supports only writes. But it can > > be easily extended to support more operations. > > > > This driver is going to be used by the monitor subsystem, which > > needs to 'emulate' a qemu-char device, so that monitor handlers > > can be ran without a backing device and have their output stored. > > > > TODO: Move buffer growth logic to cutils. > > Would be nice to have this TODO as comment in the code. True. > > Signed-off-by: Luiz Capitulino > > --- > > qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qemu-char.h | 6 +++++ > > 2 files changed, 74 insertions(+), 0 deletions(-) > > > > diff --git a/qemu-char.c b/qemu-char.c > > index 6d2dce7..1ca9ccc 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > > return NULL; > > } > > > > +/***********************************************************/ > > +/* Buffered chardev */ > > +typedef struct { > > + size_t outbuf_size; > > + size_t outbuf_capacity; > > + uint8_t *outbuf; > > +} BufferedDriver; > > Out of curiosity: how do you think input should work? Second buffer? > Loopback to same buffer? I was thinking in having a second buffer. > Maybe the buffer should be a separate data type, with the character > device type wrapped around its buffer(s). I'm not asking you to do that > now. Seems interesting. > > + > > +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > > +{ > > + BufferedDriver *d = chr->opaque; > > + > > + if (d->outbuf_capacity < (d->outbuf_size + len)) { > > Superfluous parenthesis around right operand of <. > > > + /* grow outbuf */ > > + d->outbuf_capacity += len; > > + d->outbuf_capacity *= 2; > > + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); > > + } > > + > > + memcpy(d->outbuf + d->outbuf_size, buf, len); > > + d->outbuf_size += len; > > + > > + return 0; > > Uh, aren't you supposed to return len here? Yes, but you're reviewing my RFC, I've posted an updated version already: http://lists.gnu.org/archive/html/qemu-devel/2010-10/msg02232.html I think most of your comments still apply, if not please say. > > +} > > + > > +void qemu_chr_init_buffered(CharDriverState *chr) > > +{ > > + BufferedDriver *d; > > + > > + d = qemu_mallocz(sizeof(BufferedDriver)); > > + d->outbuf_capacity = 4096; > > + d->outbuf = qemu_mallocz(d->outbuf_capacity); > > + > > + memset(chr, 0, sizeof(*chr)); > > + chr->opaque = d; > > + chr->chr_write = buffered_chr_write; > > +} > > Unlike normal character drivers, this one isn't accessible via > qemu_chr_open(). That's why you have qemu_chr_init_buffered() rather > than qemu_chr_open_buffered(). Yes, and it's simpler that way. > > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > > +{ > > + char *str; > > + QString *qs; > > + BufferedDriver *d = chr->opaque; > > + > > + if (d->outbuf_size == 0) { > > + return NULL; > > + } > > This is weird. Shouldn't we return an empty QString when chr contains > an empty string? Yeah, will fix. > > + str = qemu_malloc(d->outbuf_size + 1); > > + memcpy(str, d->outbuf, d->outbuf_size); > > + str[d->outbuf_size] = '\0'; > > + > > + qs = qstring_from_str(str); > > + qemu_free(str); > > + > > + return qs; > > +} > > While a QString is exactly what you need in PATCH 2/2, it's rather > special. Let's split it into the elementary building blocks: > > (1) Find the string stored within the chr. > (2) Copy it to a newly malloc'ed buffer. > (3) Wrap a QString around it. Already exists: qstring_from_str(). > > Maybe you'd prefer to keep (1) and (2) fused. Fine with me. This function is different in v1, it's quite simple, but it still returns a QString: /* assumes the stored data is a string */ QString *qemu_chr_buffered_to_qs(CharDriverState *chr) { BufferedDriver *d = chr->opaque; if (d->outbuf_size == 0) { return NULL; } return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); } I'd like to keep things as simple as possible for now, maybe future users will want to get a raw buffer, maybe not. Let's add it when needed. > > + > > +void qemu_chr_close_buffered(CharDriverState *chr) > > +{ > > + BufferedDriver *d = chr->opaque; > > + > > + qemu_free(d->outbuf); > > + qemu_free(chr->opaque); > > + chr->opaque = NULL; > > + chr->chr_write = NULL; > > +} > > Unlike normal character drivers, this one can't be closed with > qemu_chr_close(), can it? What happens if someone calls > qemu_chr_close() on it? I guess it will explode, because this driver is not in the chardevs list and our CharDriverState instance is allocated on the stack. Does a function comment solves the problem or do you have something else in mind? > > > + > > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > > { > > char host[65], port[33], width[8], height[8]; > > diff --git a/qemu-char.h b/qemu-char.h > > index 18ad12b..4467641 100644 > > --- a/qemu-char.h > > +++ b/qemu-char.h > > @@ -6,6 +6,7 @@ > > #include "qemu-option.h" > > #include "qemu-config.h" > > #include "qobject.h" > > +#include "qstring.h" > > > > /* character device */ > > > > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); > > > > extern int term_escape_char; > > > > +/* buffered chardev */ > > +void qemu_chr_init_buffered(CharDriverState *chr); > > +void qemu_chr_close_buffered(CharDriverState *chr); > > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); > > + > > /* async I/O support */ > > > > int qemu_set_fd_handler2(int fd, >