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

[Alexander Graf]

On 14.07.2011, at 10:58, Chunyan Liu wrote:

> Add "tee" backend to char device. It could be used as follows:
>    -serial tee:filepath,pty
>    -chardev tee,tee_fpath=path,tee_backend=pty,,path=path,,[mux=on|off]
> With "tee" option, "pty" output would be duplicated to filepath.
> Related thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00105.html
> 
> V2 changes:
>    -implement "tee" as a new backend. V1 implemented "tee" as a option.
>    -add documentation in qemu-options.hx.
> 
> Please review. Thanks.
> 
> ---
> qemu-char.c     |  168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> qemu-config.c   |    6 ++
> qemu-options.hx |   25 ++++++++-
> 3 files changed, 197 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index fb13b28..99e49a9 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -228,6 +228,156 @@ 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);
> +}
> +
> +/* tee_chr_write will return the write result of basechr, write result to 
> file
> + * will be ignored. FIX ME. */
> +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);
> +    }
> +    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(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    TeeDriver *d;
> +    CharDriverState *basechr;
> +    CharDriverState *filechr;
> +    const char *label = qemu_opts_id(opts);
> +    const char *tee_fpath = qemu_opt_get(opts, "tee_fpath");
> +    const char *tee_backend = qemu_opt_get(opts, "tee_backend");
> +    char *new_label, *new_filename;
> +    int sz;
> +
> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    d = qemu_mallocz(sizeof(TeeDriver));
> +
> +    sz = strlen(label)+3;
> +    new_label = qemu_malloc(sz);
> +    snprintf(new_label, sz, "%s-0", label);
> +    basechr = qemu_chr_open(new_label, tee_backend, NULL);
> +
> +    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);
> +
> +    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 +2506,16 @@ 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_backend", filename);
> +        qemu_opt_set(opts, "tee_fpath", tee_fpath);
> +        qemu_opt_set(opts, "backend", "tee");
> +        return opts;
> +   }
> +
>     if (strcmp(filename, "null")    == 0 ||
>         strcmp(filename, "pty")     == 0 ||
>         strcmp(filename, "msmouse") == 0 ||
> @@ -2468,6 +2628,7 @@ static const struct {
>     const char *name;
>     CharDriverState *(*open)(QemuOpts *opts);
> } backend_table[] = {
> +    { .name = "tee",       .open = qemu_chr_open_tee },
>     { .name = "null",      .open = qemu_chr_open_null },
>     { .name = "socket",    .open = qemu_chr_open_socket },
>     { .name = "udp",       .open = qemu_chr_open_udp },
> @@ -2536,7 +2697,12 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
> 
>     if (!chr->filename)
>         chr->filename = qemu_strdup(qemu_opt_get(opts, "backend"));
> -    chr->init = init;
> +    if (strcmp(qemu_opt_get(opts, "backend"), "tee") == 0) {
> +        TeeDriver *s = chr->opaque;
> +        s->basechr->init = init;

So this one is necessary because the char driver doesn't get the init function 
as a parameter to do with it as it pleases, but it gets it injected after 
open() by the char layer. That is bad.

However, I couldn't find any user of this init function. Why is it there in the 
first place? If we need it, please create a patch before this patch (just make 
it a patch set) that moves this code into the open() call of the respective 
char backend, by passing init to open(). If it's really completely unused as 
I'm seeing it atm, get rid of the whole thing altogether :)

> +    } else {
> +        chr->init = init;
> +    }
>     QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> 
>     if (qemu_opt_get_bool(opts, "mux", 0)) {
> diff --git a/qemu-config.c b/qemu-config.c
> index c63741c..b82516f 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -151,6 +151,12 @@ static QemuOptsList qemu_chardev_opts = {
>         },{
>             .name = "debug",
>             .type = QEMU_OPT_NUMBER,
> +        },{
> +            .name = "tee_backend",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "tee_fpath",
> +            .type = QEMU_OPT_STRING,
>         },
>         { /* end of list */ }
>     },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index e6d7adc..1496f34 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1428,6 +1428,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
> #if defined(CONFIG_SPICE)
>     "-chardev spicevmc,id=id,name=name[,debug=debug]\n"
> #endif
> +    "-chardev tee,id=id,tee_backend=dev_string,tee_fpath=path\n"
>     , QEMU_ARCH_ALL
> )
> 
> @@ -1453,7 +1454,8 @@ Backend is one of:
> @option{braille},
> @option{tty},
> @option{parport},
> address@hidden
> address@hidden,
> address@hidden
> The specific backend will determine the applicable options.
> 
> All devices must have an id, which can be any string up to 127 characters 
> long.
> @@ -1639,6 +1641,18 @@ required.
> Connect to a spice virtual machine channel, such as vdiport.
> #endif
> 
> address@hidden -chardev tee ,address@hidden ,address@hidden ,address@hidden
> +
> address@hidden any kind of char device specified above. Double each comma
> +in device string to ensure the whole device string is the option value, "id" 
> in
> +device string could be ignored. For example:
> +tee_backend=stdio,,mux=on,,signal=on
> +
> address@hidden file path where char device output content is duplicated to.
> +
> +Connect to a device specified by dev_string, and duplicate output content of

dev_string?

> +that device to a file.
> +
> @end table
> ETEXI
> 
> @@ -1878,6 +1892,15 @@ A unix domain socket is used instead of a tcp socket.  
> The option works the
> same as if you had specified @code{-serial tcp} except the unix domain socket
> @var{path} is used for connections.
> 
> address@hidden tee:@var{path},@var{dev_string}
> +Tee can duplicate output content of a serial device to a file.
> address@hidden is the file path where output content is duplicated to.
> address@hidden should be any one of the serial devices specified
> +above. An example would be:
> address@hidden @code
> address@hidden -serial tee:/var/log/test.log,pty

Would this still work? I thought the new syntax needs all the subdevice 
information in tee_backend?


Alex