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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 12 Nov 2010 11:16:54 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> 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.
>
> That's how you interpret it, my interpretation is that I might not get
> the expected behavior.

Actually, this function works just fine for embedded '\0's (I tested
it): it returns the correct QString, with full length and '\0' embedded.

Only later, when we attempt to put that QString on the wire do we screw
up, in to_json().  It fails to consider the length, and stops at the
first 0.  In fact, there's not even a way to get the length of a
QString!  There's only qstring_get_str().  I'd call that an API bug.
You might call it a restriction instead ;)

If anything needs a comment, it's qobject_to_json().  But I think that
one needs a bug fix instead.

Alternatively, we could document that QString and its users can't cope
with embedded '\0'.

>> 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?
>
> Should be in the json parser, but qemu_chr_mem_to_qs() doesn't assume its
> users (and it obviously shouldn't).

It's in to_json().

[...]