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<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?
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