[Qemu-devel] [RFC 0/2]: QMP: Human Monitor passthrough

[Qemu-devel] [RFC 0/2]: QMP: Human Monitor passthrough

[Luiz Capitulino]

This is my first try on implementing HMP passthrough via QMP. I've tried
to follow Anthony's advice on creating a buffered char device.

If this approach is good (and the implementation is not faulty), I believe
this is very near a mergeble state (although I haven't tested much yet).

Example:

-> { "execute": "hmp_passthrough", "arguments": { "command-line": "print /i 10+25" } }
<- { "return": "35\r\n" }

Thanks.

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

[Luiz Capitulino]

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.

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.

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;
+
+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)) {
+        /* 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;
+}
+
+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;
+}
+
+QString *qemu_chr_buffered_to_qs(CharDriverState *chr)
+{
+    char *str;
+    QString *qs;
+    BufferedDriver *d = chr->opaque;
+
+    if (d->outbuf_size == 0) {
+        return NULL;
+    }
+
+    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;
+}
+
+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;
+}
+
 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,
-- 
1.7.3.2.90.gd4c43

[Qemu-devel] [PATCH 2/2] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

This command allows QMP clients to execute HMP commands, please
check its documentation in the hmp-commands.hx file for usage
information.

Please, also note that not all HMP commands can be executed this
way, in special commands that:

 o need to store monitor related data (eg. getfd)
 o read data from the user (eg. cont when the block device is
   encrypted)

TODO: Create a blacklist for those bad commands or just let
      them fail? (assuming they won't blowup, of course)
TODO: Maybe a command like 'cpu' requires a blacklist

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   34 ++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 260cc02..a0df098 100644
--- a/monitor.c
+++ b/monitor.c
@@ -490,6 +490,40 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
+static void handle_user_command(Monitor *mon, const char *cmdline);
+
+static void hmp_call(Monitor *mon, const char *cmdline)
+{
+    Monitor *old_mon = cur_mon;
+
+    cur_mon = mon;
+    handle_user_command(mon, cmdline);
+    cur_mon = old_mon;
+}
+
+static int do_hmp_passthrough(Monitor *mon, const QDict *params,
+                              QObject **ret_data)
+{
+    QString *qs;
+    Monitor hmp;
+    CharDriverState bufchr;
+
+    memset(&hmp, 0, sizeof(hmp));
+    hmp.chr = &bufchr;
+    qemu_chr_init_buffered(hmp.chr);
+
+    hmp_call(&hmp, qdict_get_str(params, "command-line"));
+
+    qs = qemu_chr_buffered_to_qs(hmp.chr);
+    if (qs) {
+        *ret_data = QOBJECT(qs);
+    }
+
+    qemu_chr_close_buffered(hmp.chr);
+
+    return 0;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..29a6048 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -761,6 +761,38 @@ Example:
 
 Note: This command must be issued before issuing any other command.
 
+EQMP
+
+    {
+        .name       = "hmp_passthrough",
+        .args_type  = "command-line:s",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hmp_passthrough,
+    },
+
+SQMP
+hmp_passthrough
+---------------
+
+Execute a Human Monitor command.
+
+Arguments: 
+
+- command-line: the command name and its arguments, just like the
+                Human Monitor's shell (json-string)
+
+Example:
+
+-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info version" } }
+<- { "return": "0.13.50\r\n" }
+
+Note: The Human Monitor is NOT a stable interface, this means that command
+      names, arguments and responses can change or be removed at ANY time.
+      Applications that rely on long term stability guarantees should NOT
+      use this command.
+
 3. Query Commands
 =================
 
-- 
1.7.3.2.90.gd4c43

Re: [Qemu-devel] [RFC 0/2]: QMP: Human Monitor passthrough

[Anthony Liguori]

On 10/25/2010 03:06 PM, Luiz Capitulino wrote:
> This is my first try on implementing HMP passthrough via QMP. I've tried
> to follow Anthony's advice on creating a buffered char device.
>
> If this approach is good (and the implementation is not faulty), I believe
> this is very near a mergeble state (although I haven't tested much yet).
>    

Looks good to me.

Regards,

Anthony Liguori

> Example:
>
> ->  { "execute": "hmp_passthrough", "arguments": { "command-line": "print /i 10+25" } }
> <- { "return": "35\r\n" }
>
> Thanks.
>
>
>    

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

[Markus Armbruster]

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.

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

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,

Re: [Qemu-devel] [PATCH 2/2] QMP: Introduce Human Monitor passthrough command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This command allows QMP clients to execute HMP commands, please
> check its documentation in the hmp-commands.hx file for usage
> information.
>
> Please, also note that not all HMP commands can be executed this
> way, in special commands that:
>
>  o need to store monitor related data (eg. getfd)

Could you explain the problem here?

>  o read data from the user (eg. cont when the block device is
>    encrypted)

The human monitor does I/O on a character device.  Your human monitor
captures output, and sends it back over QMP.  Input could be sent over
QMP, too.  Just not interactively, as all of the input needs to be sent
along with the command.

What can't work is funky terminal stuff such as readline, but the human
monitor already degrades gracefully when run on a non-terminal character
device, doesn't it?

>
> TODO: Create a blacklist for those bad commands or just let
>       them fail? (assuming they won't blowup, of course)

We need to make sure "bad" commands fail cleanly anyway, so things don't
explode when we get the blacklist slightly wrong.

> TODO: Maybe a command like 'cpu' requires a blacklist

What's the problem with "cpu"?

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c       |   34 ++++++++++++++++++++++++++++++++++
>  qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 260cc02..a0df098 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -490,6 +490,40 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
>      return 0;
>  }
>  
> +static void handle_user_command(Monitor *mon, const char *cmdline);
> +
> +static void hmp_call(Monitor *mon, const char *cmdline)
> +{
> +    Monitor *old_mon = cur_mon;
> +
> +    cur_mon = mon;
> +    handle_user_command(mon, cmdline);
> +    cur_mon = old_mon;
> +}

Do you expect more users of this function?

> +
> +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> +                              QObject **ret_data)
> +{
> +    QString *qs;
> +    Monitor hmp;
> +    CharDriverState bufchr;
> +
> +    memset(&hmp, 0, sizeof(hmp));
> +    hmp.chr = &bufchr;
> +    qemu_chr_init_buffered(hmp.chr);
> +
> +    hmp_call(&hmp, qdict_get_str(params, "command-line"));
> +
> +    qs = qemu_chr_buffered_to_qs(hmp.chr);
> +    if (qs) {
> +        *ret_data = QOBJECT(qs);
> +    }

If the command produces no output, qemu_chr_buffered_to_qs() returns
null (which I don't like, see there), and *ret_data remains untouched.
Works for me.

> +
> +    qemu_chr_close_buffered(hmp.chr);
> +
> +    return 0;
> +}
> +
>  static int compare_cmd(const char *name, const char *list)
>  {
>      const char *p, *pstart;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 793cf1c..29a6048 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -761,6 +761,38 @@ Example:
>  
>  Note: This command must be issued before issuing any other command.
>  
> +EQMP
> +
> +    {
> +        .name       = "hmp_passthrough",
> +        .args_type  = "command-line:s",
> +        .params     = "",
> +        .help       = "",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hmp_passthrough,
> +    },
> +
> +SQMP
> +hmp_passthrough

The temptation to call this "human_passthrough" would be well-nigh
irresistible for me... can't resist cheap puns ;)

> +---------------
> +
> +Execute a Human Monitor command.
> +
> +Arguments: 
> +
> +- command-line: the command name and its arguments, just like the
> +                Human Monitor's shell (json-string)
> +
> +Example:
> +
> +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info version" } }
> +<- { "return": "0.13.50\r\n" }
> +
> +Note: The Human Monitor is NOT a stable interface, this means that command
> +      names, arguments and responses can change or be removed at ANY time.
> +      Applications that rely on long term stability guarantees should NOT
> +      use this command.
> +
>  3. Query Commands
>  =================

I'm pleasantly surprised how self-contained and simple this feature
turned out to be.  Nice work!

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

[Luiz Capitulino]

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.

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

I think most of your comments still apply, if not please say.

> > +}
> > +
> > +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 */
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?

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

Re: [Qemu-devel] [PATCH 2/2] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

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

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This command allows QMP clients to execute HMP commands, please
> > check its documentation in the hmp-commands.hx file for usage
> > information.
> >
> > Please, also note that not all HMP commands can be executed this
> > way, in special commands that:

This changed a bit in v1, care to review it instead? I will answer your
questions but skip the code comments.

> >
> >  o need to store monitor related data (eg. getfd)
> 
> Could you explain the problem here?

IIUC, we store received file-descriptors in the Monitor struct, however the
passthrough is totally stateless so we don't have where to store them.

> >  o read data from the user (eg. cont when the block device is
> >    encrypted)
> 
> The human monitor does I/O on a character device.  Your human monitor
> captures output, and sends it back over QMP.  Input could be sent over
> QMP, too.  Just not interactively, as all of the input needs to be sent
> along with the command.
> 
> What can't work is funky terminal stuff such as readline, but the human
> monitor already degrades gracefully when run on a non-terminal character
> device, doesn't it?

Yes. We could make this kind of command work by adding an input buffer to
the MemoryDriver, but I'm not sure if the additional complexity is worth it.

> > TODO: Create a blacklist for those bad commands or just let
> >       them fail? (assuming they won't blowup, of course)
> 
> We need to make sure "bad" commands fail cleanly anyway, so things don't
> explode when we get the blacklist slightly wrong.

Sure, as far as I could test it, no command will explode, if it happens
it's a bug of course.

> > TODO: Maybe a command like 'cpu' requires a blacklist
> 
> What's the problem with "cpu"?

Fixed in v1.

> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c       |   34 ++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 260cc02..a0df098 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -490,6 +490,40 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
> >      return 0;
> >  }
> >  
> > +static void handle_user_command(Monitor *mon, const char *cmdline);
> > +
> > +static void hmp_call(Monitor *mon, const char *cmdline)
> > +{
> > +    Monitor *old_mon = cur_mon;
> > +
> > +    cur_mon = mon;
> > +    handle_user_command(mon, cmdline);
> > +    cur_mon = old_mon;
> > +}
> 
> Do you expect more users of this function?
> 
> > +
> > +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> > +                              QObject **ret_data)
> > +{
> > +    QString *qs;
> > +    Monitor hmp;
> > +    CharDriverState bufchr;
> > +
> > +    memset(&hmp, 0, sizeof(hmp));
> > +    hmp.chr = &bufchr;
> > +    qemu_chr_init_buffered(hmp.chr);
> > +
> > +    hmp_call(&hmp, qdict_get_str(params, "command-line"));
> > +
> > +    qs = qemu_chr_buffered_to_qs(hmp.chr);
> > +    if (qs) {
> > +        *ret_data = QOBJECT(qs);
> > +    }
> 
> If the command produces no output, qemu_chr_buffered_to_qs() returns
> null (which I don't like, see there), and *ret_data remains untouched.
> Works for me.
> 
> > +
> > +    qemu_chr_close_buffered(hmp.chr);
> > +
> > +    return 0;
> > +}
> > +
> >  static int compare_cmd(const char *name, const char *list)
> >  {
> >      const char *p, *pstart;
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 793cf1c..29a6048 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -761,6 +761,38 @@ Example:
> >  
> >  Note: This command must be issued before issuing any other command.
> >  
> > +EQMP
> > +
> > +    {
> > +        .name       = "hmp_passthrough",
> > +        .args_type  = "command-line:s",
> > +        .params     = "",
> > +        .help       = "",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_hmp_passthrough,
> > +    },
> > +
> > +SQMP
> > +hmp_passthrough
> 
> The temptation to call this "human_passthrough" would be well-nigh
> irresistible for me... can't resist cheap puns ;)
> 
> > +---------------
> > +
> > +Execute a Human Monitor command.
> > +
> > +Arguments: 
> > +
> > +- command-line: the command name and its arguments, just like the
> > +                Human Monitor's shell (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info version" } }
> > +<- { "return": "0.13.50\r\n" }
> > +
> > +Note: The Human Monitor is NOT a stable interface, this means that command
> > +      names, arguments and responses can change or be removed at ANY time.
> > +      Applications that rely on long term stability guarantees should NOT
> > +      use this command.
> > +
> >  3. Query Commands
> >  =================
> 
> I'm pleasantly surprised how self-contained and simple this feature
> turned out to be.  Nice work!
> 

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

[Markus Armbruster]

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.

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

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

[Luiz Capitulino]

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

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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:
[...]
>> >> 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.

Not if we formalize the notion of an "internal use only" character
device.  Say, !chr->filename means it's internal, and internal ones
aren't in chardevs.  Make qemu_chr_close()'s QTAILQ_REMOVE() conditional
!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.

I doubt the alloc/free matters.

> Really worth it?

Your call.

But if you decide not to, please add a suitable assertion to
qemu_chr_close(), to make it obvious what went wrong when an internal
character device explodes there.

>> Exceptions prove the rule.  Maybe this is one, maybe not.
[...]

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

[Luiz Capitulino]

On Wed, 10 Nov 2010 14:33:47 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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:
> [...]
> >> >> 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.
> 
> Not if we formalize the notion of an "internal use only" character
> device.  Say, !chr->filename means it's internal, and internal ones
> aren't in chardevs.  Make qemu_chr_close()'s QTAILQ_REMOVE() conditional
> !chr->filename.

Yes, it's doable. But this kind of change will make this series intrusive,
for example, is it really impossible to create !chr->filename via the
normal means (eg. from the user)? What if we break something else with this
change?

> > 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.
> 
> I doubt the alloc/free matters.
> 
> > Really worth it?
> 
> Your call.

I don't think so, unless we have a real need for it (and this can be done
later anyway).

> But if you decide not to, please add a suitable assertion to
> qemu_chr_close(), to make it obvious what went wrong when an internal
> character device explodes there.
> 
> >> Exceptions prove the rule.  Maybe this is one, maybe not.
> [...]
>