[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] qemu-char: Introduce Buffered driver
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qemu-char: Introduce Buffered driver |
Date: |
Wed, 10 Nov 2010 13:56:39 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Luiz Capitulino <address@hidden> writes:
> On Wed, 10 Nov 2010 10:26:15 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> 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 <address@hidden>
>> > ---
>> > 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,
>>