Patchwork [V3] Support logging xen-guest console

login
register
mail settings
Submitter Chunyan Liu
Date June 30, 2011, 7:08 a.m.
Message ID <1309417722-11424-1-git-send-email-cyliu@novell.com>
Download mbox | patch
Permalink /patch/102712/
State New
Headers show

Comments

Chunyan Liu - June 30, 2011, 7:08 a.m.
Add code to support logging xen-domU console, as what xenconsoled does. To
enable logging, set environment variable XENCONSOLED_TRACE=guest and
XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in <specified
directory>.

Signed-off-by: Chunyan Liu <cyliu@novell.com>
---
 hw/xen_console.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)
Alexander Graf - June 30, 2011, 7:53 a.m.
On 30.06.2011, at 09:08, Chunyan Liu wrote:

> Add code to support logging xen-domU console, as what xenconsoled does. To
> enable logging, set environment variable XENCONSOLED_TRACE=guest and
> XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in <specified
> directory>.

So far, qemu doesn't rely on environment variables and I don't see a compelling reason to introduce us relying on them here. Keep in mind that with Xenner, this code is also used without Xen as the host, so it's just like any other device driver in qemu.

Please change it to command line options. Also, please run checkpatch.pl on your patch before sending it :).


Alex
Alexander Graf - June 30, 2011, 7:58 a.m.
On 30.06.2011, at 09:08, Chunyan Liu wrote:

> Add code to support logging xen-domU console, as what xenconsoled does. To
> enable logging, set environment variable XENCONSOLED_TRACE=guest and
> XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in <specified
> directory>.

In fact, this whole thing looks as if you're merely trying to reimplement "tee" on the xenconsole output. Wouldn't it make more sense to do this in the char layer? So we could do:

  -xenconsole tee:stdio,file:/tmp/xen.log

or similar? That's probably a lot more useful than a random Xen specific hack.


Alex
Chunyan Liu - June 30, 2011, 9:39 a.m.
On Thursday, June 30, 2011 03:58:57 PM Alexander Graf wrote:
> On 30.06.2011, at 09:08, Chunyan Liu wrote:
> > Add code to support logging xen-domU console, as what xenconsoled does.
> > To enable logging, set environment variable XENCONSOLED_TRACE=guest and
> > XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in
> > <specified directory>.
> 
> In fact, this whole thing looks as if you're merely trying to reimplement
> "tee" on the xenconsole output. Wouldn't it make more sense to do this in
> the char layer? So we could do:
> 
>   -xenconsole tee:stdio,file:/tmp/xen.log
> 
> or similar? That's probably a lot more useful than a random Xen specific
> hack.
> 
Thanks, Alex.  It IS something like "tee". But IMO, change in xen_console.c 
and change in char layer are just different time points, do not have essential 
difference. Change in xen_console.c is trying to backup output data into log 
file before sending to char device, change in char device is trying to 
dupicate data from char device to log file. Correct me if I'm wrong.

Thanks,
Chunyan
Alexander Graf - June 30, 2011, 11:18 a.m.
On 06/30/2011 11:39 AM, Chun Yan Liu wrote:
> On Thursday, June 30, 2011 03:58:57 PM Alexander Graf wrote:
>> On 30.06.2011, at 09:08, Chunyan Liu wrote:
>>> Add code to support logging xen-domU console, as what xenconsoled does.
>>> To enable logging, set environment variable XENCONSOLED_TRACE=guest and
>>> XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in
>>> <specified directory>.
>> In fact, this whole thing looks as if you're merely trying to reimplement
>> "tee" on the xenconsole output. Wouldn't it make more sense to do this in
>> the char layer? So we could do:
>>
>>    -xenconsole tee:stdio,file:/tmp/xen.log
>>
>> or similar? That's probably a lot more useful than a random Xen specific
>> hack.
>>
> Thanks, Alex.  It IS something like "tee". But IMO, change in xen_console.c
> and change in char layer are just different time points, do not have essential
> difference. Change in xen_console.c is trying to backup output data into log
> file before sending to char device, change in char device is trying to
> dupicate data from char device to log file. Correct me if I'm wrong.

Sure, the outcome is the same though, no? We get the output data in both 
a file and the char backend.


Alex
Chunyan Liu - July 1, 2011, 3:20 a.m.
On Thursday, June 30, 2011 07:18:54 PM you wrote:
> On 06/30/2011 11:39 AM, Chun Yan Liu wrote:
> > On Thursday, June 30, 2011 03:58:57 PM Alexander Graf wrote:
> >> On 30.06.2011, at 09:08, Chunyan Liu wrote:
> >>> Add code to support logging xen-domU console, as what xenconsoled does.
> >>> To enable logging, set environment variable XENCONSOLED_TRACE=guest and
> >>> XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in
> >>> <specified directory>.
> >> 
> >> In fact, this whole thing looks as if you're merely trying to
> >> reimplement "tee" on the xenconsole output. Wouldn't it make more sense
> >> to do this in
> >> 
> >> the char layer? So we could do:
> >>    -xenconsole tee:stdio,file:/tmp/xen.log
> >> 
> >> or similar? That's probably a lot more useful than a random Xen specific
> >> hack.
> > 
> > Thanks, Alex.  It IS something like "tee". But IMO, change in
> > xen_console.c and change in char layer are just different time points,
> > do not have essential difference. Change in xen_console.c is trying to
> > backup output data into log file before sending to char device, change
> > in char device is trying to dupicate data from char device to log file.
> > Correct me if I'm wrong.
> 
> Sure, the outcome is the same though, no? We get the output data in both
> a file and the char backend.
>  
Char device in qemu is not only used by console. Compared with the benefits 
that brings, I still doubt if it is proper to adding this functionality to 
char layer.  
Stefano, how do you think?

Thanks,
Chunyan
Stefano Stabellini - July 1, 2011, 6 p.m.
On Fri, 1 Jul 2011, Chun Yan Liu wrote:
> On Thursday, June 30, 2011 07:18:54 PM you wrote:
> > On 06/30/2011 11:39 AM, Chun Yan Liu wrote:
> > > On Thursday, June 30, 2011 03:58:57 PM Alexander Graf wrote:
> > >> On 30.06.2011, at 09:08, Chunyan Liu wrote:
> > >>> Add code to support logging xen-domU console, as what xenconsoled does.
> > >>> To enable logging, set environment variable XENCONSOLED_TRACE=guest and
> > >>> XENCONSOLED_LOGDIR=<specified directory>, log file will be saved in
> > >>> <specified directory>.
> > >> 
> > >> In fact, this whole thing looks as if you're merely trying to
> > >> reimplement "tee" on the xenconsole output. Wouldn't it make more sense
> > >> to do this in
> > >> 
> > >> the char layer? So we could do:
> > >>    -xenconsole tee:stdio,file:/tmp/xen.log
> > >> 
> > >> or similar? That's probably a lot more useful than a random Xen specific
> > >> hack.
> > > 
> > > Thanks, Alex.  It IS something like "tee". But IMO, change in
> > > xen_console.c and change in char layer are just different time points,
> > > do not have essential difference. Change in xen_console.c is trying to
> > > backup output data into log file before sending to char device, change
> > > in char device is trying to dupicate data from char device to log file.
> > > Correct me if I'm wrong.
> > 
> > Sure, the outcome is the same though, no? We get the output data in both
> > a file and the char backend.
> >  
> Char device in qemu is not only used by console. Compared with the benefits 
> that brings, I still doubt if it is proper to adding this functionality to 
> char layer.  
> Stefano, how do you think?
 
I agree with Alexander that Qemu should take a command line argument to
enable xen_console logging, rather than reading an environmental
variable.
However considering that xen_console allocates char drivers dynamically,
the command line syntax proposed by Alexander is not a good fit.
We need a global command line parameter that enables logging for all
xen_console ptys, for example:

-xencon_logs /path/to/logdir

Regarding the particular way to implement logging, I think this patch is
OK. Of course if the char layer had a "tee" option we could use it to
log the pty output, for example prepending "tee:/log/to/file," to the
filename parameter that we are going to pass to qemu_chr_open.

Patch

diff --git a/hw/xen_console.c b/hw/xen_console.c
index c6c8163..51e2ba2 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,17 @@  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 +203,51 @@  static void xencons_send(struct XenConsole *con)
     }
 }
 
+static int create_domain_log(struct XenConsole *con)
+{
+    char *logfile;
+    char *path, *domname;
+    int fd;
+    char *logdir;
+
+    path = xs_get_domain_path(xenstore, con->xendev.dom);
+    domname = xenstore_read_str(path, "name");
+    free(path);
+    if (!domname)
+        return -1;
+
+    logdir = getenv("XENCONSOLED_LOGDIR");
+    if (!logdir)
+        return -1;
+    if (mkdir(logdir, 0755) && errno != EEXIST)
+    {
+        xen_be_printf(&con->xendev, 1,
+                      "Directory %s does not exist and fail to create it!",
+                      logdir);
+        return -1;
+    }
+
+    asprintf(&logfile, "%s/guest-%s.log", logdir, 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 +266,11 @@  static int con_init(struct XenDevice *xendev)
     else
         con->chr = serial_hds[con->xendev.dev];
 
+    logenv = getenv("XENCONSOLED_TRACE");
+    if (logenv != NULL && strlen(logenv) == strlen("guest")
+               && !strcmp(logenv, "guest")) {
+        log_guest = 1;
+    }
     return 0;
 }
 
@@ -230,6 +303,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 +321,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)