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

[Markus Armbruster]

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.

What about "memory driver"?  "membuf driver"?  "string driver"?

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

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

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.

> +
> +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?

> +}
> +
> +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().

> +
> +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?

> +
> +    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.

> +
> +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?

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