qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver
Date: Fri, 12 Nov 2010 11:16:54 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux)

Luiz Capitulino <address@hidden> writes:

> On Thu, 11 Nov 2010 17:32:06 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Thu, 11 Nov 2010 16:30:26 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Luiz Capitulino <address@hidden> writes:
>> >> 
>> >> > This driver handles in-memory chardev operations. That's, all writes
>> >> > to this driver are stored in an internal buffer and it doesn't talk
>> >> > to the external world in any way.
>> >> >
>> >> > Right now it's very simple: it supports only writes. But it can be
>> >> > easily extended to support more operations.
>> >> >
>> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
>> >> > feature, which needs to run monitor handlers without a backing
>> >> > device.
>> >> >
>> >> > Signed-off-by: Luiz Capitulino <address@hidden>
>> >> > ---
>> >> >  qemu-char.c |   66 
>> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  qemu-char.h |    6 +++++
>> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
>> >> >
>> >> > diff --git a/qemu-char.c b/qemu-char.c
>> >> > index 88997f9..896df14 100644
>> >> > --- a/qemu-char.c
>> >> > +++ b/qemu-char.c
>> >> > @@ -2275,6 +2275,72 @@ static CharDriverState 
>> >> > *qemu_chr_open_socket(QemuOpts *opts)
>> >> >      return NULL;
>> >> >  }
>> >> >  
>> >> > +/***********************************************************/
>> >> > +/* Memory chardev */
>> >> > +typedef struct {
>> >> > +    size_t outbuf_size;
>> >> > +    size_t outbuf_capacity;
>> >> > +    uint8_t *outbuf;
>> >> > +} MemoryDriver;
>> >> > +
>> >> > +static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int 
>> >> > len)
>> >> > +{
>> >> > +    MemoryDriver *d = chr->opaque;
>> >> > +
>> >> > +    /* TODO: the QString implementation has the same code, we should
>> >> > +     * introduce a generic way to do this in cutils.c */
>> >> > +    if (d->outbuf_capacity < d->outbuf_size + len) {
>> >> > +        /* grown outbuf */
>> >> 
>> >> Used to say "grow" (sans n) here.  Intentional change?
>> >
>> > Hum, no. I think I've squashed an older commit while rebasing (but this 
>> > seems
>> > to be the only problem).
>> >
>> >> 
>> >> > +        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 len;
>> >> > +}
>> >> > +
>> >> > +#define DEFAULT_BUF_SIZE 4096
>> >> 
>> >> It's the *initial* buffer size, isn't it?
>> >
>> > Yes.
>> 
>> Could we make the name reflect that then?
>> 
>> >> Doubt it's worth a #define (there's just one user), but that's a matter
>> >> of taste.
>> >> 
>> >> > +
>> >> > +void qemu_chr_init_mem(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *d;
>> >> > +
>> >> > +    d = qemu_malloc(sizeof(*d));
>> >> > +    d->outbuf_size = 0;
>> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
>> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
>> >> > +
>> >> > +    memset(chr, 0, sizeof(*chr));
>> >> > +    chr->opaque = d;
>> >> > +    chr->chr_write = mem_chr_write;
>> >> > +}
>> >> > +
>> >> > +/* assumes the stored data is a string */
>> >> 
>> >> What else could it be?  Worrying about embedded '\0's?
>> >
>> > Yes, as the driver itself doesn't interpret the contents of its
>> > buffer.
>> 
>> What happens if there are embedded '\0's?
>
> The string will be shorter than expected? And what if it contains
> non-printable characters?
>
> It's just a cautionary comment to help the user identify such problems, I 
> think
> we're making a whole argument about a quite minor thing.

When I see "assumes X" in a function comment, I immediately ask "and
what happens when !X?"  The default answer is "it explodes, so don't do
that".  That answer is wrong here.  Therefore, I find the comment
misleading.

Let's figure out what really happens.  The human command's output is
sent to the client as a JSON string (response object member return).
JSON strings can consist of Unicode characters, "except for the
characters that must be escaped: quotation mark, reverse solidus, and
the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5).

Do we escape these characters?  Where in the code?

>> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *d = chr->opaque;
>> >> > +
>> >> > +    if (d->outbuf_size == 0) {
>> >> > +        return NULL;
>> >> > +    }
>> >> 
>> >> Did you forget to change this?  We agreed to return an empty QString
>> >> when chr contains an empty string.
>> >
>> > I've changed my mind and forgot to mention it: I thought that we would
>> > need to return NULL on error conditions, but turns out that this function
>> > never fails.
>> >
>> > So, I do think it's better to let it that way for two reasons:
>> >
>> >  1. An empty has at least the '\0' character, but in this case the buffer
>> >     is really empty
>> 
>> qstring_from_substr() copies the contents of the buffer (any length
>> works, including 0), then appends a '\0'.  I'm afraid I don't get the
>> problem here...
>> 
>> >  2. Returning an empty string for this case will add unneeded complexity
>> >     to the caller, ie. checking if the QString's length is 0 and 
>> > decref'ing it
>> 
>> I strongly recommend not to screw up the interface of a generally useful
>> function like qemu_chr_mem_to_qs() just to make its initial user
>> marginally simpler.
>
> Okay, found a different way of doing this that should make us both happy.
>
> Very exciting changes in v3!
>
>> 
>> If you decide not to follow my recommendation, please document the
>> unusual mapping of empty string to null pointer in a function comment.
>> 
>> >> > +
>> >> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 
>> >> > 1);
>> >> > +}
>> >> > +
>> >> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
>> >> > +void qemu_chr_close_mem(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *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().  It probably explodes if you try.  Please add a
>> >> suitable assertion to qemu_chr_close() to document the fact, and to
>> >> ensure misuse fails in a controlled, obvious manner.
>> >
>> > Ah forgot, but that can be done as a separate patch, so if I don't respin
>> > this series I'll send an additional patch for that.
>> 
>> Okay.
>
> Btw, what should we assert() for? We're going to have to access QTAILQ
> member I guess.

Fair question.

Semantically, we assert that close is safe and does the job.

For your memory driver, it's not safe, because the QTAILQ_REMOVE() is
safe only when chr is in chardevs, which it isn't.  And it doesn't do
the job, because it doesn't free resources.

We can detect the "not safe" condition: search chardevs for chr.  Might
want to put it in a function qemu_chr_is_internal().

If we don't want to search, we can add a flag that reflects "is in
chardevs".

Taking a step back: "external" character devices are in chardevs, and
are to be closed with qemu_chr_close().  "internal" ones are not, and
are to be closed differently.

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]