| Message ID | 1450353308-2082-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 17, 2015 at 01:00:41PM +0100, Paolo Bonzini wrote: > > > On 17/12/2015 12:55, Daniel P. Berrange wrote: > > Typically a UNIX guest OS will log boot messages to a serial > > port in addition to any graphical console. An admin user > > may also wish to use the serial port for an interactive > > console. A virtualization management system may wish to > > collect system boot messages by logging the serial port, > > but also wish to allow admins interactive access. > > > > Currently providing such a feature forces the mgmt app > > to either provide 2 separate serial ports, one for > > logging boot messages and one for interactive console > > login, or to proxy all output via a separate service > > that can multiplex the two needs onto one serial port. > > While both are valid approaches, they each have their > > own downsides. The former causes confusion and extra > > setup work for VM admins creating disk images. The latter > > places an extra burden to re-implement much of the QEMU > > chardev backends logic in libvirt or even higher level > > mgmt apps and adds extra hops in the data transfer path. > > > > A simpler approach that is satisfactory for many use > > cases is to allow the QEMU chardev backends to have a > > "logfile" property associated with them. > > > > $QEMU -chardev socket,host=localhost,port=9000,\ > > server=on,nowait,id-charserial0,\ > > logfile=/var/log/libvirt/qemu/test-serial0.log > > -device isa-serial,chardev=charserial0,id=serial0 > > Why for socket only? It would be very useful for stdio and especially vc. This is just an "RFC" so didn't want to spend the time to wire up all of them until I got positive feedback on the idea :-) If its acceptable, then I think it could make sense to just support optional "logfile" on every single backend, except "file" Regards, Daniel
On 17/12/2015 13:02, Daniel P. Berrange wrote: >>> > > >>> > > $QEMU -chardev socket,host=localhost,port=9000,\ >>> > > server=on,nowait,id-charserial0,\ >>> > > logfile=/var/log/libvirt/qemu/test-serial0.log >>> > > -device isa-serial,chardev=charserial0,id=serial0 >> > >> > Why for socket only? It would be very useful for stdio and especially vc. > This is just an "RFC" so didn't want to spend the time to wire > up all of them until I got positive feedback on the idea :-) Yes, I think it's fine. I guess one could do weird things with it such as connecting it to a named pipe, but we shouldn't waste time preventing it. > If its acceptable, then I think it could make sense to just > support optional "logfile" on every single backend, except "file" Yeah, I'd even wire it in qemu_chr_fe_write, and only prevent it for "file" at option parsing time. Or just allow it for all of them. Paolo
On Thu, Dec 17, 2015 at 01:49:32PM +0100, Paolo Bonzini wrote: > > > On 17/12/2015 13:02, Daniel P. Berrange wrote: > >>> > > > >>> > > $QEMU -chardev socket,host=localhost,port=9000,\ > >>> > > server=on,nowait,id-charserial0,\ > >>> > > logfile=/var/log/libvirt/qemu/test-serial0.log > >>> > > -device isa-serial,chardev=charserial0,id=serial0 > >> > > >> > Why for socket only? It would be very useful for stdio and especially vc. > > This is just an "RFC" so didn't want to spend the time to wire > > up all of them until I got positive feedback on the idea :-) > > Yes, I think it's fine. I guess one could do weird things with it such > as connecting it to a named pipe, but we shouldn't waste time preventing it. Great, I'll work on a properly complete patch for this then. > > If its acceptable, then I think it could make sense to just > > support optional "logfile" on every single backend, except "file" > > Yeah, I'd even wire it in qemu_chr_fe_write, and only prevent it for > "file" at option parsing time. Or just allow it for all of them. Yeah, doing it above the backend driver layer sounds like a nice simple approach to making it work universally. Regards, Daniel
diff --git a/qapi-schema.json b/qapi-schema.json index 8b1a423..c20fe88 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3056,6 +3056,7 @@ # then attempt a reconnect after the given number of seconds. # Setting this to zero disables this function. (default: 0) # (Since: 2.2) +# @logfile: #optional file to log all data output to (since 2.6) # # Since: 1.4 ## @@ -3064,7 +3065,8 @@ '*wait' : 'bool', '*nodelay' : 'bool', '*telnet' : 'bool', - '*reconnect' : 'int' } } + '*reconnect' : 'int', + '*logfile' : 'str' } } ## # @ChardevUdp: diff --git a/qemu-char.c b/qemu-char.c index 2969c44..a10d32e 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2537,6 +2537,7 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd) typedef struct { GIOChannel *chan, *listen_chan; + int logfd; guint listen_tag; int fd, listen_fd; int connected; @@ -2630,23 +2631,47 @@ static int unix_send_msgfds(CharDriverState *chr, const uint8_t *buf, int len) } #endif +static size_t fullwrite(int fd, const uint8_t *buf, size_t len) +{ + size_t done = 0; + ssize_t ret; + + while (done < len) { + ret = write(fd, buf + done, len - done); + if (ret < 0) + break; + done += ret; + } + + return ret; +} + /* Called with chr_write_lock held. */ static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len) { TCPCharDriver *s = chr->opaque; + int ret; if (s->connected) { #ifndef _WIN32 if (s->is_unix && s->write_msgfds_num) { - return unix_send_msgfds(chr, buf, len); + ret = unix_send_msgfds(chr, buf, len); } else #endif { - return io_channel_send(s->chan, buf, len); + ret = io_channel_send(s->chan, buf, len); } } else { /* XXX: indicate an error ? */ - return len; + ret = len; + } + + if (ret > 0) { + /* Ignoring errors from writing to logfile, as logs are + * defined to be "best effort" only */ + fullwrite(s->logfd, buf, ret); } + + return ret; } static int tcp_chr_read_poll(void *opaque) @@ -3085,6 +3110,9 @@ static void tcp_chr_close(CharDriverState *chr) if (s->write_msgfds_num) { g_free(s->write_msgfds); } + if (s->logfd != -1) { + close(s->logfd); + } g_free(s); qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } @@ -3575,6 +3603,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, const char *path = qemu_opt_get(opts, "path"); const char *host = qemu_opt_get(opts, "host"); const char *port = qemu_opt_get(opts, "port"); + const char *logfile = qemu_opt_get(opts, "logfile"); SocketAddress *addr; if (!path) { @@ -3600,6 +3629,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, backend->u.socket->wait = is_waitconnect; backend->u.socket->has_reconnect = true; backend->u.socket->reconnect = reconnect; + backend->u.socket->has_logfile = logfile != NULL; + backend->u.socket->logfile = g_strdup(logfile); addr = g_new0(SocketAddress, 1); if (path) { @@ -4041,6 +4072,9 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "chardev", .type = QEMU_OPT_STRING, + },{ + .name = "logfile", + .type = QEMU_OPT_STRING, }, { /* end of list */ } }, @@ -4201,6 +4235,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, s->fd = -1; s->listen_fd = -1; + s->logfd = -1; s->is_unix = addr->type == SOCKET_ADDRESS_KIND_UNIX; s->is_listen = is_listen; s->is_telnet = is_telnet; @@ -4219,6 +4254,16 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, /* be isn't opened until we get a connection */ chr->explicit_be_open = true; + if (sock->has_logfile) { + s->logfd = qemu_open(sock->logfile, O_WRONLY | O_CREAT | O_TRUNC, 0666); + if (s->logfd < 0) { + error_setg_errno(errp, errno, + "Unable to open logfile %s", + sock->logfile); + goto error; + } + } + chr->filename = g_malloc(CHR_MAX_FILENAME_SIZE); SocketAddress_to_str(chr->filename, CHR_MAX_FILENAME_SIZE, "disconnected:", addr, is_listen, is_telnet); @@ -4234,10 +4279,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, if (s->reconnect_time) { socket_try_connect(chr); } else if (!qemu_chr_open_socket_fd(chr, errp)) { - g_free(s); - g_free(chr->filename); - g_free(chr); - return NULL; + goto error; } if (is_listen && is_waitconnect) { @@ -4248,6 +4290,15 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, } return chr; + + error: + if (s->logfd != -1) { + close(s->logfd); + } + g_free(s); + g_free(chr->filename); + g_free(chr); + return NULL; } static CharDriverState *qmp_chardev_open_udp(const char *id,
Typically a UNIX guest OS will log boot messages to a serial port in addition to any graphical console. An admin user may also wish to use the serial port for an interactive console. A virtualization management system may wish to collect system boot messages by logging the serial port, but also wish to allow admins interactive access. Currently providing such a feature forces the mgmt app to either provide 2 separate serial ports, one for logging boot messages and one for interactive console login, or to proxy all output via a separate service that can multiplex the two needs onto one serial port. While both are valid approaches, they each have their own downsides. The former causes confusion and extra setup work for VM admins creating disk images. The latter places an extra burden to re-implement much of the QEMU chardev backends logic in libvirt or even higher level mgmt apps and adds extra hops in the data transfer path. A simpler approach that is satisfactory for many use cases is to allow the QEMU chardev backends to have a "logfile" property associated with them. $QEMU -chardev socket,host=localhost,port=9000,\ server=on,nowait,id-charserial0,\ logfile=/var/log/libvirt/qemu/test-serial0.log -device isa-serial,chardev=charserial0,id=serial0 Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- qapi-schema.json | 4 +++- qemu-char.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 8 deletions(-)