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

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