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

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

[Stefano Stabellini]

On Thu, 7 Jul 2011, 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?

Configure your Xen HVM guest to use the serial.

Compile Qemu as emulator:

./configure --disable-xen --target-list=x86_64-softmmu

then use qemu directly to boot your guest:

./qemu-system-x86_64 -hda /path/to/file -serial tree:/path/to/log,pty

you can connect to the pty using a program like picocom:

picocom -b 115200 /dev/pts/$NUM

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<cyliu@novell.com>
> ---
>   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

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

[Chun Yan Liu]

On Thursday, July 07, 2011 09:51:45 PM you wrote:
> 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<cyliu@novell.com>
> > ---
> > 
> >   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?
Result of file write is ignored, we cann't do anything except printing some 
error log, and we don't want char device output is affacted by that result. 
So, still write to char device. How do you think? 
> 
> > +    }
> > +    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 :).

I have thought of implementing it as a new backend. But I reviewed "mux" code, 
it's implemented as an option. So, ..

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

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

[Alexander Graf]

On 08.07.2011, at 12:17, Chun Yan Liu wrote:

> On Thursday, July 07, 2011 09:51:45 PM you wrote:
>> 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<cyliu@novell.com>
>>> ---
>>> 
>>>  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?
> Result of file write is ignored, we cann't do anything except printing some 
> error log, and we don't want char device output is affacted by that result. 
> So, still write to char device. How do you think? 

Well, we could repeat the write at a later point or escalate up that the write failed. We could even go as far as failing the write altogether if the file write failed. The return value is "number of bytes written" btw, so there could be a case where we send a write of 10 bytes in, the file backend writes 5 bytes and the other backend writes 7.

But I guess we should just ignore these corner cases and put a comment in the code saying that we're aware of the potential issues :).

>> 
>>> +    }
>>> +    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 :).
> 
> I have thought of implementing it as a new backend. But I reviewed "mux" code, 
> it's implemented as an option. So, ..

Ugh, I figured you might say that. Please ignore the mux target. It's ugly and hacky and been a constant source of pain :). It's very useful in practice, but you don't need all those hacks for a tee target.


Alex