Luiz Capitulino <lcapitul...@redhat.com> writes: > On Wed, 10 Nov 2010 10:26:15 +0100 > Markus Armbruster <arm...@redhat.com> wrote: > >> Luiz Capitulino <lcapitul...@redhat.com> 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.
Works for me. >> > 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 <lcapitul...@redhat.com> >> > --- >> > 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 Meh. Sorry about that. > I think most of your comments still apply, if not please say. Okay to simply review your respin when it comes? >> > +} >> > + >> > +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 */ What else could it be? Worrying about embedded '\0's? > 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? A general OO rule: Having different constructors for different sub-types is okay, but once constructed, you should be able to use the objects without knowing of what sub-type they are. That includes destruction. Exceptions prove the rule. Maybe this is one, maybe not. >> > + >> > 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, >>