Re: [Qemu-devel] [PATCH 1/2] qemu-char: Introduce Buffered driver

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 10 Nov 2010 13:56:39 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 10 Nov 2010 10:26:15 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@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 <lcapitulino@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.

We will have to add our MemoryDriver to the chardevs list, this has some
implications like being visible in qemu_chr_info() and qemu_chr_find(),
likely to also imply that we should choose a chr->filename.

Another detail is that we'll have to dynamically alocate our CharDriverState
instance. Not a problem, but adds a few more lines of code and a
qemu_free(). None of this is needed today.

Really worth it?

> 
> 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,
> >> 
>