|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] tools/xenconsoled: Log guest serial consoles
On Tue, 2013-07-02 at 20:42 +0100, Andrew Cooper wrote:
> The ability to log guest consoles can now be controlled using xenstore keys
> under /local/logconsole.
You appear to be implementing a completely orthogonal scheme to the
existing guest logging stuff. The thing which your scheme gets us is
more dynamic control over which domains to log when and the ability for
the user to pick the path, rather than simply having it
be /var/log/xen/console/dom%d.log (or is it domname, I didn't check).
If we think those benefits are worthwhile (I suppose they are) then I do
think they need to be integrated with the existing stuff, i.e. by
manipulating dom->log_fd rather than by adding a second FILE * to struct
domain.
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> --
> This has been used extensivly in the XenServer automated testing
> infrastructure for many years, and interest has been expressed in having the
> functionality upstream.
> ---
> docs/misc/xenstore-paths.markdown | 15 +++++++
> tools/console/daemon/io.c | 82
> +++++++++++++++++++++++++++++++++++++
> tools/console/daemon/io.h | 1 +
> tools/console/daemon/main.c | 2 +
> 4 files changed, 100 insertions(+)
>
> diff --git a/docs/misc/xenstore-paths.markdown
> b/docs/misc/xenstore-paths.markdown
> index 1c634b5..c2de6d1 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -348,6 +348,21 @@ The time which the guest was started in
> SECONDS.MICROSECONDS format
>
> The guest's virtual time offset from UTC in seconds.
>
> +## Logging paths
> +
> +### /local/logconsole/$DOMID = PATH [n,INTERNAL]
> +
> +Write a path to this key to enable console logging for the specified domain.
> +Writing an empty string or removing the node causes logging to stop.
> +Rewriting the path causes the daemon to close and reopen the file, which can
> +be uses to rotate the log file.
> +
> +### /local/logconsole/@ = PATH [n,INTERNAL]
> +
> +Wildcard logging path, for domains without a `/local/logconsole/$DOMID` path.
> +The path must contain "%d" which shall be substituted with the domid, and
> must
> +not contain any other "%" characters.
Looking at the implementation won't writing this node override any
previously existing /local/logconsole/$DOMID which is in force?
Also, I don't see how rewriting it causes all domains to update the own
file handle, so they will keep logging to the original, won't they? Only
new domains would pickup the new path.
Likewise it doesn't appear to be possible to remove or clear this node
to disable logging.
> ## Platform-Level paths
>
> ### libxl Specific Paths
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 250550a..86314c4 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -103,8 +103,12 @@ struct domain {
> struct xencons_interface *interface;
> int event_count;
> long long next_period;
> + FILE *logfile;
> };
>
> +static void update_logconsole(struct domain *);
> +static char *wildcard_logfile = NULL;
> +
> static struct domain *dom_head;
>
> static int write_all(int fd, const char* buf, size_t len)
> @@ -158,6 +162,7 @@ static void buffer_append(struct domain *dom)
> struct buffer *buffer = &dom->buffer;
> XENCONS_RING_IDX cons, prod, size;
> struct xencons_interface *intf = dom->interface;
> + size_t begin;
>
> cons = intf->out_cons;
> prod = intf->out_prod;
> @@ -176,9 +181,15 @@ static void buffer_append(struct domain *dom)
> }
> }
>
> + begin = buffer->size;
> while (cons != prod)
> buffer->data[buffer->size++] = intf->out[
> MASK_XENCONS_IDX(cons++, intf->out)];
> + if (dom->logfile && buffer->size != begin) {
> + fwrite(&buffer->data[begin], buffer->size - begin, 1,
> + dom->logfile);
> + fflush(dom->logfile);
> + }
Does this correctly handle wrapping around at the end of the ring?
>
> xen_mb();
> intf->out_cons = cons;
> @@ -683,6 +694,9 @@ static struct domain *create_domain(int domid)
> if (!watch_domain(dom, true))
> goto out;
>
> + dom->logfile = NULL;
> + update_logconsole(dom);
> +
> dom->next = dom_head;
> dom_head = dom;
>
> @@ -714,6 +728,9 @@ static void remove_domain(struct domain *dom)
> for (pp = &dom_head; *pp; pp = &(*pp)->next) {
> if (dom == *pp) {
> *pp = dom->next;
> + if (dom->logfile)
> + fclose(dom->logfile);
> + dom->logfile = NULL;
> free(dom);
> break;
> }
> @@ -773,6 +790,37 @@ static void enum_domains(void)
> }
> }
>
> +static void update_logconsole(struct domain *dom)
> +{
> + char *fname = NULL, *path = NULL;
> + FILE *oldfile;
> +
> + oldfile = dom->logfile;
> +
> + if (asprintf(&path, "/local/logconsole/%d", dom->domid) == -1)
> + goto out;
> +
> + fname = xs_read(xs, XBT_NULL, path, NULL);
> + if (!fname && wildcard_logfile)
> + if (asprintf(&fname, wildcard_logfile, dom->domid) == -1)
> + goto out;
I'm not 100% convinced that reopening the same file (even in append
mode) is the right thing to do, O_APPEND certainly has caveats wrt
multiple processes. I suppose xenconsoled is single threaded though, so
it is safe enough.
It would be safer (or at least more obviously correct) to open the
wildcard logfile once and writes were directed explicitly there. This
would also solve some of the issue WRT rewriting the control node.
> + if (!fname || !fname[0])
> + goto out;
> +
> + dom->logfile = fopen(fname, "a");
> + if (!dom->logfile)
> + dolog(LOG_ERR, "fopen %s failed", fname);
> +
> + out:
> + if (oldfile && dom->logfile == oldfile) {
> + dom->logfile = NULL;
> + fclose(oldfile);
> + }
> + free(fname);
> + free(path);
> + return;
> +}
> +
> static int ring_free_bytes(struct domain *dom)
> {
> struct xencons_interface *intf = dom->interface;
> @@ -896,6 +944,30 @@ static void handle_xs(void)
> been removed, so dom may be NULL here. */
> if (dom && dom->is_dead == false)
> domain_create_ring(dom);
> + } else if (!strcmp(vec[XS_WATCH_TOKEN], "logconsole")) {
> + if (sscanf(vec[XS_WATCH_PATH], "/local/logconsole/%u",
> + &domid) == 1) {
> + dom = lookup_domain(domid);
> + if (dom && dom->is_dead == false)
> + update_logconsole(dom);
> + } else if (!strcmp(vec[XS_WATCH_PATH],
> + "/local/logconsole/@")) {
> + char *wildcard, *tmp;
> + free(wildcard_logfile);
> + wildcard_logfile = NULL;
> + wildcard = xs_read(xs, XBT_NULL,
> + "/local/logconsole/@", NULL);
> + /* Sanitise string, as it gets used by asprintf(). It
> + * should contain exactly one "%d" and no futher "%"s */
> + if(wildcard) {
> + tmp = strchr(wildcard, '%');
> + if(tmp && tmp[1] == 'd' &&
> + strchr(&tmp[1], '%') == NULL)
> + wildcard_logfile = wildcard;
> + else
> + free(wildcard);
> + }
> + }
> }
>
> free(vec);
> @@ -1197,6 +1269,16 @@ void handle_io(void)
> log_hv_evtchn = -1;
> }
>
> +void watch_logconsole(void)
> +{
> + bool success;
> +
> + success = xs_watch(xs, "/local/logconsole", "logconsole");
> + if (!success)
> + dolog(LOG_ERR, "logconsole watch failed");
> + wildcard_logfile = xs_read(xs, XBT_NULL, "/local/logconsole/@", NULL);
But you don't open it here?
> +}
> +
> /*
> * Local variables:
> * c-file-style: "linux"
> diff --git a/tools/console/daemon/io.h b/tools/console/daemon/io.h
> index f658bfc..96e08b4 100644
> --- a/tools/console/daemon/io.h
> +++ b/tools/console/daemon/io.h
> @@ -22,5 +22,6 @@
> #define CONSOLED_IO_H
>
> void handle_io(void);
> +void watch_logconsole(void);
>
> #endif
> diff --git a/tools/console/daemon/main.c b/tools/console/daemon/main.c
> index 92d2fc4..8229162 100644
> --- a/tools/console/daemon/main.c
> +++ b/tools/console/daemon/main.c
> @@ -161,6 +161,8 @@ int main(int argc, char **argv)
> if (!xen_setup())
> exit(1);
>
> + watch_logconsole();
> +
> handle_io();
>
> closelog();
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |