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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 12 Nov 2010 11:21:57 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  qemu-char.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qemu-char.h |    7 ++++++
> >  2 files changed, 77 insertions(+), 0 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 88997f9..36d23c6 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -2275,6 +2275,76 @@ 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) {
> > +        /* 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 len;
> > +}
> > +
> > +void qemu_chr_init_mem(CharDriverState *chr)
> > +{
> > +    MemoryDriver *d;
> > +
> > +    d = qemu_malloc(sizeof(*d));
> > +    d->outbuf_size = 0;
> > +    d->outbuf_capacity = 4096;
> > +    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 */
> 
> This could indicate a problem.  See my reply in the thread for v2.

Replied, but I can't see the problem.

> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> > +{
> > +    MemoryDriver *d = chr->opaque;
> > +
> > +    if (d->outbuf_size == 0) {
> > +        return qstring_new();
> > +    }
> 
> Why is this necessary?  Is qstring_from_substr() broken for empty
> substrings?  If it is, it ought to be fixed!

qstring_from_substr() takes a character range; outbuf_size stores a size,
not a string length. So we do:

> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);

If outbuf_size is 0, we'll be passing a negative value down.

> > +}
> > +
> > +/* 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;
> > +}
> > +
> > +size_t qemu_chr_mem_osize(const CharDriverState *chr)
> > +{
> > +    const MemoryDriver *d = chr->opaque;
> > +    return d->outbuf_size;
> > +}
> > +
> >  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..e6ee6c4 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,12 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
> >  
> >  extern int term_escape_char;
> >  
> > +/* memory chardev */
> > +void qemu_chr_init_mem(CharDriverState *chr);
> > +void qemu_chr_close_mem(CharDriverState *chr);
> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
> > +size_t qemu_chr_mem_osize(const CharDriverState *chr);
> > +
> >  /* async I/O support */
> >  
> >  int qemu_set_fd_handler2(int fd,
>