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

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

[Chun Yan Liu]

[-- Attachment #1.1: Type: text/plain, Size: 1571 bytes --]

Alex & Anthony,

About this issue, how should we proceed?

Chunyan

>>> Anthony Liguori  07/24/11 9:25 PM >>>
On 07/24/2011 04:47 AM, Alexander Graf wrote:
>
>> These arguments all apply to any possible option.  Why not a grep target?  Why not a cut or less target?
>
> Because they don't make sense.

Neither does tee :-)

>>> As long as the tee target is reasonably isolated, I don't think it'd clutter the char backend.
>>
>> It'll never be tested and end up becoming dead bloat code.
>
> I'll be extensively tested in Xen code, since that's how Xen will invoke qemu.

If this is being used by Xen as part of Xend, then there's really no 
point in worrying about complexity because the user will never be 
exposed to it.

>
> I tend to agree, but this time the solution in qemu is cleaner and easier IMHO.

Let's compare the typical use-case:

qemu -hda linux.img -nographic | tee boot.log

vs:

qemu -hda linux.img -serial tee:boot.log,stdio

I'm not even sure I got the syntax right on the later.

> Also, I've had issues with tee several times already.

Then we should fix those issues.  But I have a hard time believing there 
were ever issues.  This is just piping.

> I'm in favor of putting a tee target into qemu's char layer and I'm sure it'll become a well used target.

It will only be used by Xen and Xen could very easily solve this problem 
outside of qemu.

It adds another twisted command line syntax that will make it harder to 
generalize later.

Regards,

Anthony Liguori

>
> Alex
>
>



[-- Attachment #1.2: Type: text/html, Size: 2033 bytes --]

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

[Anthony Liguori]

On 08/04/2011 09:26 PM, Chun Yan Liu wrote:
> Alex&  Anthony,
>
> About this issue, how should we proceed?

Do the logging somewhere else in the Xen toolchain.

Regards,

Anthony Liguori

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

[Anthony Liguori]

On 07/24/2011 04:47 AM, Alexander Graf wrote:
>
>> These arguments all apply to any possible option.  Why not a grep target?  Why not a cut or less target?
>
> Because they don't make sense.

Neither does tee :-)

>>> As long as the tee target is reasonably isolated, I don't think it'd clutter the char backend.
>>
>> It'll never be tested and end up becoming dead bloat code.
>
> I'll be extensively tested in Xen code, since that's how Xen will invoke qemu.

If this is being used by Xen as part of Xend, then there's really no 
point in worrying about complexity because the user will never be 
exposed to it.

>
> I tend to agree, but this time the solution in qemu is cleaner and easier IMHO.

Let's compare the typical use-case:

qemu -hda linux.img -nographic | tee boot.log

vs:

qemu -hda linux.img -serial tee:boot.log,stdio

I'm not even sure I got the syntax right on the later.

> Also, I've had issues with tee several times already.

Then we should fix those issues.  But I have a hard time believing there 
were ever issues.  This is just piping.

> I'm in favor of putting a tee target into qemu's char layer and I'm sure it'll become a well used target.

It will only be used by Xen and Xen could very easily solve this problem 
outside of qemu.

It adds another twisted command line syntax that will make it harder to 
generalize later.

Regards,

Anthony Liguori

>
> Alex
>
>

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

[Alexander Graf]

On 23.07.2011, at 21:11, Anthony Liguori wrote:

> On 07/23/2011 01:31 PM, Alexander Graf wrote:
>> 
>> On 23.07.2011, at 17:23, Anthony Liguori wrote:
>> 
>>> On 07/14/2011 03:58 AM, 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
>>> 
>>> I loathe adding even more complexity to the the char layer.  Can't you do this just as well with socat?
>> 
>> I disagree. For socat we'd have to open some listening port (unix, tcg, etc) and then have socat connect to it.
> 
> And what's the problem?  Use a unix domain socket and call it a day.
> 
>> While socat is not up yet, the VM won't run. It also adds another layer of complexity (syncing of socat and qemu process) to the picture that I don't like.
> 
> These arguments all apply to any possible option.  Why not a grep target?  Why not a cut or less target?

Because they don't make sense.

>> As long as the tee target is reasonably isolated, I don't think it'd clutter the char backend.
> 
> It'll never be tested and end up becoming dead bloat code.

I'll be extensively tested in Xen code, since that's how Xen will invoke qemu.

> For uncommon use cases where there's another way to do something with no real obvious technical advantages, using existing code (and utilities) always wins vs reinventing the wheel IMHO.

I tend to agree, but this time the solution in qemu is cleaner and easier IMHO. Also, I've had issues with tee several times already. In fact, I actually tried to use tee plenty of times to log console and debug output to a file while working with Qemu and it randomly just stopped processing data. While that could be a bug somewhere in one of the random layers, it shows me how bad complexity grows if you add even more programs to the puzzle that you can't rely on.

I'm in favor of putting a tee target into qemu's char layer and I'm sure it'll become a well used target.


Alex

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},
> -@option{spicevmc}.
> +@option{spicevmc},
> +@option{tee}.
> 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
> 
> +@item -chardev tee ,id=@var{id} ,tee_backend=@var{dev_string} ,tee_fpath=@var{path}
> +
> +@option{tee_backend} 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
> +
> +@option{tee_fpath} 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.
> 
> +@item tee:@var{path},@var{dev_string}
> +Tee can duplicate output content of a serial device to a file.
> +@var{path} is the file path where output content is duplicated to.
> +@var{dev_string} should be any one of the serial devices specified
> +above. An example would be:
> +@table @code
> +@item -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

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

[Anthony Liguori]

On 07/23/2011 01:31 PM, Alexander Graf wrote:
>
> On 23.07.2011, at 17:23, Anthony Liguori wrote:
>
>> On 07/14/2011 03:58 AM, 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
>>
>> I loathe adding even more complexity to the the char layer.  Can't you do this just as well with socat?
>
> I disagree. For socat we'd have to open some listening port (unix, tcg, etc) and then have socat connect to it.

And what's the problem?  Use a unix domain socket and call it a day.

> While socat is not up yet, the VM won't run. It also adds another layer of complexity (syncing of socat and qemu process) to the picture that I don't like.

These arguments all apply to any possible option.  Why not a grep 
target?  Why not a cut or less target?

> As long as the tee target is reasonably isolated, I don't think it'd clutter the char backend.

It'll never be tested and end up becoming dead bloat code.

For uncommon use cases where there's another way to do something with no 
real obvious technical advantages, using existing code (and utilities) 
always wins vs reinventing the wheel IMHO.

Regards,

Anthony Liguori

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

[Alexander Graf]

On 23.07.2011, at 17:23, Anthony Liguori wrote:

> On 07/14/2011 03:58 AM, 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
> 
> I loathe adding even more complexity to the the char layer.  Can't you do this just as well with socat?

I disagree. For socat we'd have to open some listening port (unix, tcg, etc) and then have socat connect to it. While socat is not up yet, the VM won't run. It also adds another layer of complexity (syncing of socat and qemu process) to the picture that I don't like.

As long as the tee target is reasonably isolated, I don't think it'd clutter the char backend. It'd be just like any other backend and eventually could maybe move into something more structured, plugin'ish, like the block layer.

I haven't gotten around to review the patch again though, will try to do soon.

Alex

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

[Anthony Liguori]

On 07/14/2011 03:58 AM, 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

I loathe adding even more complexity to the the char layer.  Can't you 
do this just as well with socat?

Regards,

Anthony Liguori

>
> 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;
> +    } 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},
> -@option{spicevmc}.
> +@option{spicevmc},
> +@option{tee}.
>   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
>
> +@item -chardev tee ,id=@var{id} ,tee_backend=@var{dev_string} ,tee_fpath=@var{path}
> +
> +@option{tee_backend} 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
> +
> +@option{tee_fpath} file path where char device output content is duplicated to.
> +
> +Connect to a device specified by dev_string, and duplicate output content of
> +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.
>
> +@item tee:@var{path},@var{dev_string}
> +Tee can duplicate output content of a serial device to a file.
> +@var{path} is the file path where output content is duplicated to.
> +@var{dev_string} should be any one of the serial devices specified
> +above. An example would be:
> +@table @code
> +@item -serial tee:/var/log/test.log,pty
> +@end table
> +
>   @item mon:@var{dev_string}
>   This is a special option to allow the monitor to be multiplexed onto
>   another serial port.  The monitor is accessed with key sequence of

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

[Chunyan Liu]

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;
+    } 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},
-@option{spicevmc}.
+@option{spicevmc},
+@option{tee}.
 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
 
+@item -chardev tee ,id=@var{id} ,tee_backend=@var{dev_string} ,tee_fpath=@var{path}
+
+@option{tee_backend} 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
+
+@option{tee_fpath} file path where char device output content is duplicated to.
+
+Connect to a device specified by dev_string, and duplicate output content of
+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.
 
+@item tee:@var{path},@var{dev_string}
+Tee can duplicate output content of a serial device to a file.
+@var{path} is the file path where output content is duplicated to.
+@var{dev_string} should be any one of the serial devices specified
+above. An example would be:
+@table @code
+@item -serial tee:/var/log/test.log,pty
+@end table
+
 @item mon:@var{dev_string}
 This is a special option to allow the monitor to be multiplexed onto
 another serial port.  The monitor is accessed with key sequence of
-- 
1.7.3.4