diff mbox

[RFC] qemu-char: add an "overlay" backend type

Message ID 1450288571-5939-1-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Dec. 16, 2015, 5:56 p.m. UTC
Introduce a new QEMU chardev backend called "overlay" which
allows you to splice together a pair of chardev backends into
one combined backend. The master backend permits full input/output
but the slave backend is output only.

The primary use case for this is to allow arbitrary backends to
have their data logged to a file, eg a 'file' backend would be
setup as the slave.

 $ qemu-system-x86_64 \
     -chardev socket,host=localhost,port=9000,server=on,nowait,id=char0master \
     -chardev file,path=/some/log/file.log,id=char0slave \
     -chardev overlay,id=char0,master=char0master,slave=char0slave  \
     -device isa-serial,chardev=char0 \
     ...other args...
---

This idea was suggsted in

  https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01256.html

this patch is a very quick proof of concept impl to illustrate the
idea.

 qapi-schema.json |  14 +++++
 qemu-char.c      | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)

Comments

Paolo Bonzini Dec. 16, 2015, 6:13 p.m. UTC | #1
On 16/12/2015 18:56, Daniel P. Berrange wrote:
> Introduce a new QEMU chardev backend called "overlay" which
> allows you to splice together a pair of chardev backends into
> one combined backend. The master backend permits full input/output
> but the slave backend is output only.
> 
> The primary use case for this is to allow arbitrary backends to
> have their data logged to a file, eg a 'file' backend would be
> setup as the slave.
> 
>  $ qemu-system-x86_64 \
>      -chardev socket,host=localhost,port=9000,server=on,nowait,id=char0master \
>      -chardev file,path=/some/log/file.log,id=char0slave \
>      -chardev overlay,id=char0,master=char0master,slave=char0slave  \
>      -device isa-serial,chardev=char0 \
>      ...other args...
> ---
> 
> This idea was suggsted in
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01256.html
> 
> this patch is a very quick proof of concept impl to illustrate the
> idea.

Hmm, I was a fan of the "do it outside QEMU" idea... It would also fix
the issue you have with qemu_chr_fe_write_all...

Paolo
Daniel P. Berrangé Dec. 17, 2015, 12:01 p.m. UTC | #2
On Wed, Dec 16, 2015 at 07:13:08PM +0100, Paolo Bonzini wrote:
> 
> 
> On 16/12/2015 18:56, Daniel P. Berrange wrote:
> > Introduce a new QEMU chardev backend called "overlay" which
> > allows you to splice together a pair of chardev backends into
> > one combined backend. The master backend permits full input/output
> > but the slave backend is output only.
> > 
> > The primary use case for this is to allow arbitrary backends to
> > have their data logged to a file, eg a 'file' backend would be
> > setup as the slave.
> > 
> >  $ qemu-system-x86_64 \
> >      -chardev socket,host=localhost,port=9000,server=on,nowait,id=char0master \
> >      -chardev file,path=/some/log/file.log,id=char0slave \
> >      -chardev overlay,id=char0,master=char0master,slave=char0slave  \
> >      -device isa-serial,chardev=char0 \
> >      ...other args...
> > ---
> > 
> > This idea was suggsted in
> > 
> >   https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01256.html
> > 
> > this patch is a very quick proof of concept impl to illustrate the
> > idea.
> 
> Hmm, I was a fan of the "do it outside QEMU" idea... It would also fix
> the issue you have with qemu_chr_fe_write_all...

I did more work on the "do it outside QEMU" idea in libvirt, but it
quickly became apparent that I'd basically end up re-implementing
all the QEMU chardev backends in libvirt, which seems sub-optimal
to me, when all I really want is a logfile added :-(

At the same time I'm also not a huge fan of this overlay backend
now that I've thought about this overnight. Aside from the issue
mentioned in the code, the other downside of this proposed patch
is that it makes life significantly more complicated for libvirt,
as when configuring QEMU we now have to create 3 chardevs instead
of just one. THis is quite alot of extra pain just for the goal
of supporting. I think this also opens a can of worms, because
I realized this allows us to stack up chardevs arbitrarily deep
by creating an overlay on an overlay on an overlay on an overlay....
which will probably come back to bite us later.

So I've sent another alternative, which simply directly adds a
"logfile" parameter to the "socket" backend, which is directly
addressing the core need that OpenStack has here (they want to
have TCP / UNIX socket for interactive console + a logfile to
capture all output on the same serial port concurrently).

Regards,
Daniel
diff mbox

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 4be151c..1adb220 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3156,6 +3156,19 @@ 
 { 'struct': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
 
 ##
+# @ChardevOverlay:
+#
+# Configuration info for overlay chardevs.
+#
+# @master: the master chardev for input / output
+# @slave: the slave chardev for output only
+#
+# Since: 2.6
+##
+{ 'struct': 'ChardevOverlay', 'data': { 'master' : 'str',
+                                        'slave'  : 'str'} }
+
+##
 # @ChardevBackend:
 #
 # Configuration info for the new chardev backend.
@@ -3182,6 +3195,7 @@ 
                                        'spiceport' : 'ChardevSpicePort',
                                        'vc'     : 'ChardevVC',
                                        'ringbuf': 'ChardevRingbuf',
+                                       'overlay': 'ChardevOverlay',
                                        # next one is just for compatibility
                                        'memory' : 'ChardevRingbuf' } }
 
diff --git a/qemu-char.c b/qemu-char.c
index 45fdb77..0328eb9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3223,6 +3223,131 @@  char *qmp_ringbuf_read(const char *device, int64_t size,
     return data;
 }
 
+
+typedef struct {
+    CharDriverState *master;
+    CharDriverState *slave;
+
+    IOCanReadHandler *chr_can_read;
+    IOReadHandler *chr_read;
+    IOEventHandler *chr_event;
+    void *handler_opaque;
+} OverlayCharDriver;
+
+static int overlay_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    OverlayCharDriver *s = chr->opaque;
+    int ret;
+
+    ret = qemu_chr_fe_write(s->master, buf, len);
+    if (ret <= 0) {
+        return ret;
+    }
+
+    /* Write all will block, but how else can we ensure
+     * that all the data we just sent to the master also
+     * gets sent to the slave ? Could buffer data internally
+     * if slave was full and setup a backgroupd I/O watch
+     * to send the data later, but the ChardevDriverState
+     * API design doesn't make that very easy since. Of
+     * course if the slave is a "file" backend, then it
+     * will block regardless thanks to POSIX not handling
+     * O_NONBLOCK on regular files, so this is no worse
+     * in that scenario.
+     */
+    qemu_chr_fe_write_all(s->slave, buf, ret);
+    return ret;
+}
+
+static GSource *overlay_chr_add_watch(CharDriverState *chr, GIOCondition cond)
+{
+    OverlayCharDriver *d = chr->opaque;
+    return d->master->chr_add_watch(d->master, cond);
+}
+
+static int overlay_chr_can_read(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    OverlayCharDriver *d = chr->opaque;
+
+    if (d->chr_can_read) {
+        return d->chr_can_read(d->handler_opaque);
+    }
+    return 0;
+}
+
+static void overlay_chr_read(void *opaque, const uint8_t *buf, int size)
+{
+    CharDriverState *chr = opaque;
+    OverlayCharDriver *d = chr->opaque;
+
+    if (d->chr_read) {
+        d->chr_read(d->handler_opaque, buf, size);
+    }
+}
+
+static void overlay_chr_event(void *opaque, int event)
+{
+    CharDriverState *chr = opaque;
+    OverlayCharDriver *d = chr->opaque;
+
+    if (d->chr_event) {
+        d->chr_event(d->handler_opaque, event);
+    }
+}
+
+static void overlay_chr_update_read_handler(CharDriverState *chr)
+{
+    OverlayCharDriver *d = chr->opaque;
+
+    d->handler_opaque = chr->handler_opaque;
+    d->chr_can_read = chr->chr_can_read;
+    d->chr_read = chr->chr_read;
+    d->chr_event = chr->chr_event;
+
+    qemu_chr_add_handlers(d->master, overlay_chr_can_read, overlay_chr_read,
+                          overlay_chr_event, chr);
+    /* Slave is output only, so don't wire it up for input */
+}
+
+static CharDriverState *qemu_chr_open_overlay(const char *id,
+                                              ChardevBackend *backend,
+                                              ChardevReturn *ret, Error **errp)
+{
+    ChardevOverlay *overlay = backend->u.overlay;
+    CharDriverState *chr, *master, *slave;
+    OverlayCharDriver *d;
+
+    master = qemu_chr_find(overlay->master);
+    if (master == NULL) {
+        error_setg(errp, "overlay: master chardev %s not found", overlay->master);
+        return NULL;
+    }
+
+    slave = qemu_chr_find(overlay->slave);
+    if (slave == NULL) {
+        error_setg(errp, "overlay: slave chardev %s not found", overlay->slave);
+        return NULL;
+    }
+
+    chr = qemu_chr_alloc();
+    d = g_new0(OverlayCharDriver, 1);
+
+    chr->opaque = d;
+    d->master = master;
+    d->slave = slave;
+
+    chr->chr_write = overlay_chr_write;
+    chr->chr_update_read_handler = overlay_chr_update_read_handler;
+    if (master->chr_add_watch) {
+        chr->chr_add_watch = overlay_chr_add_watch;
+    }
+    chr->explicit_be_open = master->explicit_be_open;
+
+    return chr;
+}
+
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
@@ -3443,6 +3568,25 @@  static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
     }
 }
 
+static void qemu_chr_parse_overlay(QemuOpts *opts, ChardevBackend *backend,
+                                   Error **errp)
+{
+    const char *master = qemu_opt_get(opts, "master");
+    const char *slave = qemu_opt_get(opts, "slave");
+
+    if (master == NULL) {
+        error_setg(errp, "chardev: overlay: no master chardev given");
+        return;
+    }
+    if (slave == NULL) {
+        error_setg(errp, "chardev: overlay: no slave chardev given");
+        return;
+    }
+    backend->u.overlay = g_new0(ChardevOverlay, 1);
+    backend->u.overlay->master = g_strdup(master);
+    backend->u.overlay->slave = g_strdup(slave);
+}
+
 static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
                                Error **errp)
 {
@@ -3943,6 +4087,12 @@  QemuOptsList qemu_chardev_opts = {
         },{
             .name = "chardev",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "master",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "slave",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -4315,6 +4465,8 @@  static void register_types(void)
     /* Bug-compatibility: */
     register_char_driver("memory", CHARDEV_BACKEND_KIND_MEMORY,
                          qemu_chr_parse_ringbuf, qemu_chr_open_ringbuf);
+    register_char_driver("overlay", CHARDEV_BACKEND_KIND_OVERLAY,
+                         qemu_chr_parse_overlay, qemu_chr_open_overlay);
     /* this must be done after machine init, since we register FEs with muxes
      * as part of realize functions like serial_isa_realizefn when -nographic
      * is specified