* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
@ 2011-08-05 2:26 Chun Yan Liu
2011-08-05 13:32 ` Anthony Liguori
0 siblings, 1 reply; 9+ messages in thread
From: Chun Yan Liu @ 2011-08-05 2:26 UTC (permalink / raw)
To: anthony, agraf; +Cc: qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
@ 2011-07-14 8:58 Chunyan Liu
2011-07-23 15:23 ` Anthony Liguori
2011-07-24 9:44 ` Alexander Graf
0 siblings, 2 replies; 9+ messages in thread
From: Chunyan Liu @ 2011-07-14 8:58 UTC (permalink / raw)
To: qemu-devel, agraf
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-14 8:58 Chunyan Liu
@ 2011-07-23 15:23 ` Anthony Liguori
2011-07-23 18:31 ` Alexander Graf
2011-07-24 9:44 ` Alexander Graf
1 sibling, 1 reply; 9+ messages in thread
From: Anthony Liguori @ 2011-07-23 15:23 UTC (permalink / raw)
To: Chunyan Liu; +Cc: qemu-devel, agraf
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-23 15:23 ` Anthony Liguori
@ 2011-07-23 18:31 ` Alexander Graf
2011-07-23 19:11 ` Anthony Liguori
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Graf @ 2011-07-23 18:31 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Chunyan Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-23 18:31 ` Alexander Graf
@ 2011-07-23 19:11 ` Anthony Liguori
2011-07-24 9:47 ` Alexander Graf
0 siblings, 1 reply; 9+ messages in thread
From: Anthony Liguori @ 2011-07-23 19:11 UTC (permalink / raw)
To: Alexander Graf; +Cc: Chunyan Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-23 19:11 ` Anthony Liguori
@ 2011-07-24 9:47 ` Alexander Graf
2011-07-24 13:25 ` Anthony Liguori
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Graf @ 2011-07-24 9:47 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Chunyan Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-24 9:47 ` Alexander Graf
@ 2011-07-24 13:25 ` Anthony Liguori
0 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2011-07-24 13:25 UTC (permalink / raw)
To: Alexander Graf; +Cc: Chunyan Liu, qemu-devel
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device
2011-07-14 8:58 Chunyan Liu
2011-07-23 15:23 ` Anthony Liguori
@ 2011-07-24 9:44 ` Alexander Graf
1 sibling, 0 replies; 9+ messages in thread
From: Alexander Graf @ 2011-07-24 9:44 UTC (permalink / raw)
To: Chunyan Liu; +Cc: qemu-devel@nongnu.org Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-08-05 13:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-05 2:26 [Qemu-devel] [PATCH V2] Add "tee" option to qemu char device Chun Yan Liu
2011-08-05 13:32 ` Anthony Liguori
-- strict thread matches above, loose matches on Subject: below --
2011-07-14 8:58 Chunyan Liu
2011-07-23 15:23 ` Anthony Liguori
2011-07-23 18:31 ` Alexander Graf
2011-07-23 19:11 ` Anthony Liguori
2011-07-24 9:47 ` Alexander Graf
2011-07-24 13:25 ` Anthony Liguori
2011-07-24 9:44 ` Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.