Re: [Qemu-devel] [PATCH] Add "tee" option to qemu char device

[Alexander Graf]

On 07/07/2011 10:24 AM, Chunyan Liu wrote:
> In previous thread "Support logging xen-guest console", it's considered that
> adding a "tee" option to char layer is a more generic way and makes more sense.
> http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg03011.html
>
> Following is an implementation of "tee" option to char device. It could be used
> as follows:
>      -chardev pty,id=id,path=path,[mux=on|off],[tee=filepath]
>      -serial tee:filepath,pty
> With "tee" option, "pty" output would be duplicated to filepath.
>
> I've ported this patch to qemu-xen and tested with xen guests already. But I'm
> not very clear how to test the qemu binary directly. Any info?
>
> Please share your comments. Thanks!
>
> Signed-off-by: Chunyan Liu<address@hidden>
> ---
>   qemu-char.c   |  159 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qemu-config.c |    3 +

This is missing documentation in *.hx

>   2 files changed, 162 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index fb13b28..7281ab4 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -228,6 +228,135 @@ static CharDriverState *qemu_chr_open_null(QemuOpts *opts)
>       return chr;
>   }
>
> +/* Tee driver */
> +typedef struct {
> +    CharDriverState *basechr; /* base io*/
> +    CharDriverState *filechr; /* duplicate output to file */
> +} TeeDriver;
> +
> +static void tee_init(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->init) {
> +        s->basechr->init(s->basechr);
> +    }
> +    if (s->filechr->init) {
> +        s->filechr->init(s->filechr);
> +    }
> +}
> +
> +static void tee_chr_update_read_handler(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    qemu_chr_add_handlers(s->basechr, chr->chr_can_read, chr->chr_read,
> +                              chr->chr_event, chr->handler_opaque);
> +}
> +
> +static int tee_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->filechr->chr_write) {
> +        s->filechr->chr_write(s->filechr, buf, len);

What would we do if the file write didn't finish?

> +    }
> +    if (s->basechr->chr_write) {
> +        return s->basechr->chr_write(s->basechr, buf, len);
> +    }
> +    return 0;
> +}
> +
> +static void tee_chr_close(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_close) {
> +        s->basechr->chr_close(s->basechr);
> +    }
> +    if (s->filechr->chr_close) {
> +        s->filechr->chr_close(s->filechr);
> +    }
> +    qemu_free(s);
> +}
> +
> +static int tee_chr_ioctl(CharDriverState *chr, int cmd, void *arg)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_ioctl) {
> +        return s->basechr->chr_ioctl(s->basechr, cmd, arg);
> +    }
> +    return 0;
> +}
> +
> +static int tee_get_msgfd(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->get_msgfd) {
> +        return s->basechr->get_msgfd(s->basechr);
> +    }
> +    return -1;
> +}
> +
> +static void tee_chr_send_event(CharDriverState *chr, int event)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_send_event) {
> +        s->basechr->chr_send_event(s->basechr, event);
> +    }
> +}
> +
> +static void tee_chr_accept_input(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_accept_input) {
> +        s->basechr->chr_accept_input(s->basechr);
> +    }
> +}
> +static void tee_chr_set_echo(CharDriverState *chr, bool echo)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_set_echo) {
> +        s->basechr->chr_set_echo(s->basechr, echo);
> +    }
> +}
> +static void tee_chr_guest_open(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_guest_open) {
> +        s->basechr->chr_guest_open(s->basechr);
> +    }
> +}
> +static void tee_chr_guest_close(CharDriverState *chr)
> +{
> +    TeeDriver *s = chr->opaque;
> +    if (s->basechr->chr_guest_close) {
> +        s->basechr->chr_guest_close(s->basechr);
> +    }
> +}
> +
> +static CharDriverState *qemu_chr_open_tee(CharDriverState *basechr,
> +                                 CharDriverState *filechr)
> +{
> +    CharDriverState *chr;
> +    TeeDriver *d;
> +
> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    d = qemu_mallocz(sizeof(TeeDriver));

Instead of having 2 allocated regions, could you please fold them 
together and access each other through DO_UPCAST?

  typedef struct {
      CharDriverState chr;      /* our own driver state */
      CharDriverState *basechr; /* base io*/
      CharDriverState *filechr; /* duplicate output to file */
  } TeeDriver;


[...]

void foo(CharDriverState *chr)
{
  TeeDriver *d = DO_UPCAST(TeeDriver, chr, chr);
  [...]

}

> +
> +    d->basechr = basechr;
> +    d->filechr = filechr;
> +    chr->opaque = d;
> +    chr->init = tee_init;
> +    chr->chr_write = tee_chr_write;
> +    chr->chr_close = tee_chr_close;
> +    chr->chr_update_read_handler = tee_chr_update_read_handler;
> +    chr->chr_ioctl = tee_chr_ioctl;
> +    chr->get_msgfd = tee_get_msgfd;
> +    chr->chr_send_event = tee_chr_send_event;
> +    chr->chr_accept_input = tee_chr_accept_input;
> +    chr->chr_set_echo = tee_chr_set_echo;
> +    chr->chr_guest_open = tee_chr_guest_open;
> +    chr->chr_guest_close = tee_chr_guest_close;
> +
> +    return chr;
> +}
>   /* MUX driver for serial I/O splitting */
>   #define MAX_MUX 4
>   #define MUX_BUFFER_SIZE 32    /* Must be a power of 2.  */
> @@ -2356,6 +2485,13 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
> char *filename)
>           qemu_opt_set(opts, "mux", "on");
>       }
>
> +    if (strstart(filename, "tee:",&p)) {
> +        char tee_fpath[1024];
> +        p = get_opt_value(tee_fpath, sizeof(tee_fpath), p);
> +        filename = p+1;
> +        qemu_opt_set(opts, "tee", tee_fpath);

This should set an function local variable to remember that it's 
actually the "tee" backend we want to use and then set "backend" to 
"tee" at the end after saving the determined "backend" option to the 
"tee_backend"(?) option.

The tee backend really shouldn't be too different from the other 
backends :).

> +    }
> +
>       if (strcmp(filename, "null")    == 0 ||
>           strcmp(filename, "pty")     == 0 ||
>           strcmp(filename, "msmouse") == 0 ||
> @@ -2506,6 +2642,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
>   {
>       CharDriverState *chr;
>       int i;
> +    const char *tee_fpath;
>
>       if (qemu_opts_id(opts) == NULL) {
>           fprintf(stderr, "chardev: no id specified\n");
> @@ -2539,6 +2676,28 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
>       chr->init = init;
>       QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>
> +    tee_fpath = qemu_opt_get(opts, "tee");

No, instead the backend should be set to "tee", so that we get into our 
own init function and can initialize the real backends from there, no? 
Namely the "file" and the earlier set backends.

> +    if (tee_fpath) {
> +        CharDriverState *basechr = chr;
> +        CharDriverState *filechr;
> +        char *new_label, *new_filename;
> +        const char *label = qemu_opts_id(opts);
> +        int sz = strlen(label)+3;
> +        basechr->label = qemu_malloc(sz);
> +        new_label = qemu_malloc(sz);
> +        snprintf(basechr->label, sz, "%s-0", label);
> +        snprintf(new_label, sz, "%s-1", label);
> +        sz = strlen(tee_fpath)+6;
> +        new_filename = qemu_malloc(sz);
> +        snprintf(new_filename, sz, "file:%s", tee_fpath);
> +        filechr = qemu_chr_open(new_label, new_filename, NULL);
> +        qemu_free(new_label);
> +        qemu_free(new_filename);
> +        chr = qemu_chr_open_tee(basechr, filechr);
> +        chr->filename = basechr->filename;
> +        QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> +    }
> +
>       if (qemu_opt_get_bool(opts, "mux", 0)) {
>           CharDriverState *base = chr;
>           int len = strlen(qemu_opts_id(opts)) + 6;
> diff --git a/qemu-config.c b/qemu-config.c
> index c63741c..fa2b430 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -151,6 +151,9 @@ static QemuOptsList qemu_chardev_opts = {
>           },{
>               .name = "debug",
>               .type = QEMU_OPT_NUMBER,
> +        },{
> +            .name = "tee",
> +            .type = QEMU_OPT_STRING,
>           },
>           { /* end of list */ }
>       },


Alex