Patchwork Support logging xen-guest console

login
register
mail settings
Submitter Chunyan Liu
Date June 20, 2011, 9:16 a.m.
Message ID <1308561383-18964-1-git-send-email-cyliu@novell.com>
Download mbox | patch
Permalink /patch/101062/
State New
Headers show

Comments

Chunyan Liu - June 20, 2011, 9:16 a.m.
Add code to support logging xen-domU console, as what xenconsoled does. Log info
will be saved in /var/log/xen/console/guest-domUname.log.

Signed-off-by: Chunyan Liu <cyliu@novell.com>
---
 hw/xen_console.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)
Stefano Stabellini - June 20, 2011, 6:42 p.m.
On Mon, 20 Jun 2011, Chunyan Liu wrote:
> Add code to support logging xen-domU console, as what xenconsoled does. Log info
> will be saved in /var/log/xen/console/guest-domUname.log.
> 
> Signed-off-by: Chunyan Liu <cyliu@novell.com>
> ---
>  hw/xen_console.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 63 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/xen_console.c b/hw/xen_console.c
> index c6c8163..ac3208d 100644
> --- a/hw/xen_console.c
> +++ b/hw/xen_console.c
> @@ -36,6 +36,8 @@
>  #include "qemu-char.h"
>  #include "xen_backend.h"
>  
> +static int log_guest = 0;
> +
>  struct buffer {
>      uint8_t *data;
>      size_t consumed;
> @@ -52,8 +54,24 @@ struct XenConsole {
>      void              *sring;
>      CharDriverState   *chr;
>      int               backlog;
> +    int               log_fd;
>  };
>  
> +static int write_all(int fd, const char* buf, size_t len)
> +{
> +    while (len) {
> +        ssize_t ret = write(fd, buf, len);
> +        if (ret == -1 && errno == EINTR)
> +            continue;
> +        if (ret <= 0)
> +            return -1;
> +        len -= ret;
> +        buf += ret;
> +    }
> +
> +    return 0;
> +}
> +

If I am not mistaken ret == 0 doesn't always mean an error on write.


>  static void buffer_append(struct XenConsole *con)
>  {
>      struct buffer *buffer = &con->buffer;
> @@ -81,6 +99,14 @@ static void buffer_append(struct XenConsole *con)
>      intf->out_cons = cons;
>      xen_be_send_notify(&con->xendev);
>  
> +    if (con->log_fd != -1) {
> +        int logret;
> +        logret = write_all(con->log_fd, buffer->data + buffer->size - size, size);
> +        if (logret < 0)
> +            xen_be_printf(&con->xendev, 1, "Write to log failed on domain %d: %d (%s)\n",
> +                      con->xendev.dom, errno, strerror(errno));
> +     }

code style: you needs brackets around the xen_be_printf statement


>      if (buffer->max_capacity &&
>  	buffer->size > buffer->max_capacity) {
>  	/* Discard the middle of the data. */
> @@ -174,12 +200,36 @@ static void xencons_send(struct XenConsole *con)
>      }
>  }
>  
> +static int create_domain_log(struct XenConsole *con)
> +{
> +    char *logfile;
> +    char *path, *domname;
> +    int fd;
> +
> +    path = xs_get_domain_path(xenstore, con->xendev.dom);
> +    domname = xenstore_read_str(path, "name");
> +    free(path);
> +    if (!domname)
> +        return -1;
> +
> +    asprintf(&logfile, "/var/log/xen/console/guest-%s.log", domname);
> +    qemu_free(domname);
> +
> +    fd = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
> +    free(logfile);
> +    if (fd == -1)
> +        xen_be_printf(&con->xendev, 1,  "Failed to open log %s: %d (%s)", logfile, errno, strerror(errno));
> +
> +    return fd;
> +}
> +

What if the "console" subdirectory is missing? Maybe we should create
the directory automatically here.


>  /* -------------------------------------------------------------------- */
>  
>  static int con_init(struct XenDevice *xendev)
>  {
>      struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
>      char *type, *dom;
> +    char *logenv = NULL;
>  
>      /* setup */
>      dom = xs_get_domain_path(xenstore, con->xendev.dom);
> @@ -198,6 +248,10 @@ static int con_init(struct XenDevice *xendev)
>      else
>          con->chr = serial_hds[con->xendev.dev];
>  
> +    logenv = getenv("XENCONSOLED_TRACE");
> +    if (logenv != NULL && !strcmp(logenv, "guest")) {
> +        log_guest = 1;
> +    }
>      return 0;
>  }

please check the length of logenv before using strcmp on it
Ian Campbell - June 23, 2011, 3:02 p.m.
On Mon, 2011-06-20 at 19:42 +0100, Stefano Stabellini wrote:
> > @@ -198,6 +248,10 @@ static int con_init(struct XenDevice *xendev)
> >      else
> >          con->chr = serial_hds[con->xendev.dev];
> >  
> > +    logenv = getenv("XENCONSOLED_TRACE");
> > +    if (logenv != NULL && !strcmp(logenv, "guest")) {
> > +        log_guest = 1;
> > +    }
> >      return 0;
> >  }
> 
> please check the length of logenv before using strcmp on it

surely getenv() isn't the correct interface anyway?

What guarantees that the environment running qemu will have
the /etc/sysconfig/xencommons derived settings in it? It's not usually
the case under xl unless the user happens to have sources the file
(which would be strange).

Also XENCONSOLED_TRACE is clearly a setting aimed at xenconsoled -- not
qemu. In fact it is really a setting for the xencommons initscript which
turns it into the appropriate xenconsoled command line option.

Perhaps we should be plumbing the console to xenconsoled (like we do for
serial = pty), that'll get us this logging for free and be consistent to
Xen users compared with PV guests, HVM serial=pty and similar...

The other option would be to properly plumb this stuff through the qemu
command line and make use of it from the Xen toolstack as necessary.

Ian.
Stefano Stabellini - June 23, 2011, 3:31 p.m.
On Thu, 23 Jun 2011, Ian Campbell wrote:
> On Mon, 2011-06-20 at 19:42 +0100, Stefano Stabellini wrote:
> > > @@ -198,6 +248,10 @@ static int con_init(struct XenDevice *xendev)
> > >      else
> > >          con->chr = serial_hds[con->xendev.dev];
> > >  
> > > +    logenv = getenv("XENCONSOLED_TRACE");
> > > +    if (logenv != NULL && !strcmp(logenv, "guest")) {
> > > +        log_guest = 1;
> > > +    }
> > >      return 0;
> > >  }
> > 
> > please check the length of logenv before using strcmp on it
> 
> surely getenv() isn't the correct interface anyway?
> 
> What guarantees that the environment running qemu will have
> the /etc/sysconfig/xencommons derived settings in it? It's not usually
> the case under xl unless the user happens to have sources the file
> (which would be strange).
> 
> Also XENCONSOLED_TRACE is clearly a setting aimed at xenconsoled -- not
> qemu. In fact it is really a setting for the xencommons initscript which
> turns it into the appropriate xenconsoled command line option.

Considering that xenconsoled and qemu consoles are equivalent and
interchangeable, it is a good idea to support XENCONSOLED_TRACE is qemu
as well.
How we do that is another matter.
It is certainly more coherent from the qemu point of view if we pass
it as a command line option, like Anthony suggested.


> Perhaps we should be plumbing the console to xenconsoled (like we do for
> serial = pty), that'll get us this logging for free and be consistent to
> Xen users compared with PV guests, HVM serial=pty and similar...

xenconsoled doesn't support any output other than a pty and doesn't
support multiple PV consoles, that are going to be required at least for
linux stubdoms.


> The other option would be to properly plumb this stuff through the qemu
> command line and make use of it from the Xen toolstack as necessary.

That would be good, maybe cleaner.

Patch

diff --git a/hw/xen_console.c b/hw/xen_console.c
index c6c8163..ac3208d 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -36,6 +36,8 @@ 
 #include "qemu-char.h"
 #include "xen_backend.h"
 
+static int log_guest = 0;
+
 struct buffer {
     uint8_t *data;
     size_t consumed;
@@ -52,8 +54,24 @@  struct XenConsole {
     void              *sring;
     CharDriverState   *chr;
     int               backlog;
+    int               log_fd;
 };
 
+static int write_all(int fd, const char* buf, size_t len)
+{
+    while (len) {
+        ssize_t ret = write(fd, buf, len);
+        if (ret == -1 && errno == EINTR)
+            continue;
+        if (ret <= 0)
+            return -1;
+        len -= ret;
+        buf += ret;
+    }
+
+    return 0;
+}
+
 static void buffer_append(struct XenConsole *con)
 {
     struct buffer *buffer = &con->buffer;
@@ -81,6 +99,14 @@  static void buffer_append(struct XenConsole *con)
     intf->out_cons = cons;
     xen_be_send_notify(&con->xendev);
 
+    if (con->log_fd != -1) {
+        int logret;
+        logret = write_all(con->log_fd, buffer->data + buffer->size - size, size);
+        if (logret < 0)
+            xen_be_printf(&con->xendev, 1, "Write to log failed on domain %d: %d (%s)\n",
+                      con->xendev.dom, errno, strerror(errno));
+     }
+
     if (buffer->max_capacity &&
 	buffer->size > buffer->max_capacity) {
 	/* Discard the middle of the data. */
@@ -174,12 +200,36 @@  static void xencons_send(struct XenConsole *con)
     }
 }
 
+static int create_domain_log(struct XenConsole *con)
+{
+    char *logfile;
+    char *path, *domname;
+    int fd;
+
+    path = xs_get_domain_path(xenstore, con->xendev.dom);
+    domname = xenstore_read_str(path, "name");
+    free(path);
+    if (!domname)
+        return -1;
+
+    asprintf(&logfile, "/var/log/xen/console/guest-%s.log", domname);
+    qemu_free(domname);
+
+    fd = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
+    free(logfile);
+    if (fd == -1)
+        xen_be_printf(&con->xendev, 1,  "Failed to open log %s: %d (%s)", logfile, errno, strerror(errno));
+
+    return fd;
+}
+
 /* -------------------------------------------------------------------- */
 
 static int con_init(struct XenDevice *xendev)
 {
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
     char *type, *dom;
+    char *logenv = NULL;
 
     /* setup */
     dom = xs_get_domain_path(xenstore, con->xendev.dom);
@@ -198,6 +248,10 @@  static int con_init(struct XenDevice *xendev)
     else
         con->chr = serial_hds[con->xendev.dev];
 
+    logenv = getenv("XENCONSOLED_TRACE");
+    if (logenv != NULL && !strcmp(logenv, "guest")) {
+        log_guest = 1;
+    }
     return 0;
 }
 
@@ -230,6 +284,9 @@  static int con_connect(struct XenDevice *xendev)
 		  con->xendev.remote_port,
 		  con->xendev.local_port,
 		  con->buffer.max_capacity);
+    con->log_fd = -1;
+    if (log_guest)
+         con->log_fd = create_domain_log(con);
     return 0;
 }
 
@@ -245,6 +302,12 @@  static void con_disconnect(struct XenDevice *xendev)
 	munmap(con->sring, XC_PAGE_SIZE);
 	con->sring = NULL;
     }
+
+    if (con->log_fd != -1) {
+        close(con->log_fd);
+        con->log_fd = -1;
+    }
+
 }
 
 static void con_event(struct XenDevice *xendev)