| Message ID | 20231101093325.30302-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: enable xenstore-stubdom to use 9pfs | expand |
On Wed, Nov 1, 2023 at 10:27 AM Juergen Gross <jgross@suse.com> wrote: > > Add "xenlogd", a new logging daemon meant to support infrastructure > domains (e.g. xenstore-stubdom) to write log files in dom0. As I understand it, your new daemon is a generic 9pfs backend, which you use for logging. I think naming it something like xen9pfsd would more accurately describe its functionality. > For now only add the code needed for starting the daemon and > registering it with Xenstore via a new "/tool/xenlog/state" node by > writing the "running" state to it. To support driver domain use cases, I think you want to use a relative Xenstore path. While this daemon is independent from libxl, it might be easiest to use "libxl/xenlog/" ("libxl/xen9pfs/") to take advantage of driver domains having a read-write "libxl/" directory. > Signed-off-by: Juergen Gross <jgross@suse.com> The code looks good to me. Regards, Jason
On 01.11.23 19:36, Jason Andryuk wrote: > On Wed, Nov 1, 2023 at 10:27 AM Juergen Gross <jgross@suse.com> wrote: >> >> Add "xenlogd", a new logging daemon meant to support infrastructure >> domains (e.g. xenstore-stubdom) to write log files in dom0. > > As I understand it, your new daemon is a generic 9pfs backend, which > you use for logging. I think naming it something like xen9pfsd would > more accurately describe its functionality. Fine with me. I'll wait a little bit to see what others think. >> For now only add the code needed for starting the daemon and >> registering it with Xenstore via a new "/tool/xenlog/state" node by >> writing the "running" state to it. > > To support driver domain use cases, I think you want to use a relative > Xenstore path. While this daemon is independent from libxl, it might > be easiest to use "libxl/xenlog/" ("libxl/xen9pfs/") to take advantage > of driver domains having a read-write "libxl/" directory. You have a point here. And as it is libxl controlling the start of the daemon, putting it under "libxl" is fine IMO. > >> Signed-off-by: Juergen Gross <jgross@suse.com> > > The code looks good to me. Thanks, Juergen
On 02/11/2023 7:44 am, Juergen Gross wrote: > On 01.11.23 19:36, Jason Andryuk wrote: >> On Wed, Nov 1, 2023 at 10:27 AM Juergen Gross <jgross@suse.com> wrote: >>> >>> Add "xenlogd", a new logging daemon meant to support infrastructure >>> domains (e.g. xenstore-stubdom) to write log files in dom0. >> >> As I understand it, your new daemon is a generic 9pfs backend, which >> you use for logging. I think naming it something like xen9pfsd would >> more accurately describe its functionality. > > Fine with me. I'll wait a little bit to see what others think. FWIW, I too looked at xenlogd and thought it probably wasn't ideal, but I was going to wait until glancing at the whole series before suggesting an alternative. If it really is a generic 9pfs backend, then +1 to Jason's suggestion, although preferably as xen-9pfsd for improved legibility. But a couple of other remarks while I'm here. It's great to see that it only uses stable libraries, and I hope that remains true to the end of the series. If we end up wanting unstable APIs, can I suggest that we take the opportunity to stabilise them as a prerequisite. In this patch, stop_me needs to be volatile or read with ACCESS_ONCE()/etc. The only thing stopping GCC turning it into an infinite loop is sleep() being an external call. daemon_running may need similar treatment, depending on how it gets used in later patches. ~Andrew
On 02.11.23 20:15, Andrew Cooper wrote: > On 02/11/2023 7:44 am, Juergen Gross wrote: >> On 01.11.23 19:36, Jason Andryuk wrote: >>> On Wed, Nov 1, 2023 at 10:27 AM Juergen Gross <jgross@suse.com> wrote: >>>> >>>> Add "xenlogd", a new logging daemon meant to support infrastructure >>>> domains (e.g. xenstore-stubdom) to write log files in dom0. >>> >>> As I understand it, your new daemon is a generic 9pfs backend, which >>> you use for logging. I think naming it something like xen9pfsd would >>> more accurately describe its functionality. >> >> Fine with me. I'll wait a little bit to see what others think. > > FWIW, I too looked at xenlogd and thought it probably wasn't ideal, but > I was going to wait until glancing at the whole series before suggesting > an alternative. > > If it really is a generic 9pfs backend, then +1 to Jason's suggestion, > although preferably as xen-9pfsd for improved legibility. Okay. > But a couple of other remarks while I'm here. > > It's great to see that it only uses stable libraries, and I hope that > remains true to the end of the series. If we end up wanting unstable > APIs, can I suggest that we take the opportunity to stabilise them as a > prerequisite. No unstable APIs. It does use xenctrl.h, but it needs that only for the cpu barriers. Maybe we should extract those to another header? > In this patch, stop_me needs to be volatile or read with > ACCESS_ONCE()/etc. The only thing stopping GCC turning it into an > infinite loop is sleep() being an external call. Okay, will use volatile. > daemon_running may > need similar treatment, depending on how it gets used in later patches. I don't think it needs special treatment. Juergen
diff --git a/tools/Makefile b/tools/Makefile index 3a510663a0..0225020416 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -32,6 +32,7 @@ SUBDIRS-y += xenpmd SUBDIRS-$(CONFIG_GOLANG) += golang SUBDIRS-y += xl SUBDIRS-y += helpers +SUBDIRS-y += xenlogd SUBDIRS-$(CONFIG_X86) += xenpaging SUBDIRS-$(CONFIG_X86) += debugger SUBDIRS-$(CONFIG_TESTS) += tests diff --git a/tools/xenlogd/.gitignore b/tools/xenlogd/.gitignore new file mode 100644 index 0000000000..a0305ae096 --- /dev/null +++ b/tools/xenlogd/.gitignore @@ -0,0 +1 @@ +/xenlogd diff --git a/tools/xenlogd/Makefile b/tools/xenlogd/Makefile new file mode 100644 index 0000000000..550e914f59 --- /dev/null +++ b/tools/xenlogd/Makefile @@ -0,0 +1,38 @@ +# +# tools/helpers/Makefile +# + +XEN_ROOT = $(CURDIR)/../.. +include $(XEN_ROOT)/tools/Rules.mk + +CFLAGS += $(PTHREAD_CFLAGS) +LDFLAGS += $(PTHREAD_LDFLAGS) + +TARGETS := xenlogd + +XENLOGD_OBJS = xenlogd.o +$(XENLOGD_OBJS): CFLAGS += $(CFLAGS_libxenstore) +$(XENLOGD_OBJS): CFLAGS += $(CFLAGS_libxenevtchn) +$(XENLOGD_OBJS): CFLAGS += $(CFLAGS_libxengnttab) +xenlogd: LDLIBS += $(call xenlibs-ldlibs,store evtchn gnttab) + +.PHONY: all +all: $(TARGETS) + +xenlogd: $(XENLOGD_OBJS) + $(CC) $(LDFLAGS) -o $@ $(XENLOGD_OBJS) $(LDLIBS) $(APPEND_LDFLAGS) + +.PHONY: install +install: all + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) + for i in $(TARGETS); do $(INSTALL_PROG) $$i $(DESTDIR)$(LIBEXEC_BIN); done + +.PHONY: uninstall +uninstall: + for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done + +.PHONY: clean +clean: + $(RM) *.o $(TARGETS) $(DEPS_RM) + +distclean: clean diff --git a/tools/xenlogd/xenlogd.c b/tools/xenlogd/xenlogd.c new file mode 100644 index 0000000000..792d1026a3 --- /dev/null +++ b/tools/xenlogd/xenlogd.c @@ -0,0 +1,145 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * xenlogd - Xen logging daemon + * + * Copyright (C) 2023 Juergen Gross <jgross@suse.com> + * + * Daemon to enable guests to access a directory of the dom0 file system. + * Access is made via the 9pfs protocol (xenlogd acts as a PV 9pfs backend). + * + * Usage: xenlogd + * + * xenlogd does NOT support writing any links (neither soft links nor hard + * links), and it is accepting only canonicalized file paths in order to + * avoid the possibility to "escape" from the guest specific directory. + * + * The backend device string is "xen_9pfs", the tag used for mounting the + * 9pfs device is "Xen". + * + * As an additional security measure the maximum file space used by the guest + * can be limited by the backend Xenstore node "max-size" specifying the size + * in MBytes. This size includes the size of the root directory of the guest. + */ + +#include <err.h> +#include <errno.h> +#include <signal.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <syslog.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <xenevtchn.h> +#include <xengnttab.h> +#include <xenstore.h> + +static bool stop_me; +static bool daemon_running; +static struct xs_handle *xs; +static xengnttab_handle *xg; +static xenevtchn_handle *xe; + +static void handle_stop(int sig) +{ + stop_me = true; +} + +static void close_all(void) +{ + if ( daemon_running ) + xs_rm(xs, XBT_NULL, "/tool/xenlog"); + if ( xe ) + xenevtchn_close(xe); + if ( xg ) + xengnttab_close(xg); + if ( xs ) + xs_close(xs); + closelog(); +} + +static void do_err(const char *msg) +{ + syslog(LOG_ALERT, "%s, errno = %d", msg, errno); + close_all(); + exit(1); +} + +static void xen_connect(void) +{ + xs_transaction_t t; + char *val; + unsigned int len; + + xs = xs_open(0); + if ( xs == NULL ) + do_err("xs_open() failed"); + + xg = xengnttab_open(NULL, 0); + if ( xg == NULL ) + do_err("xengnttab_open() failed"); + + xe = xenevtchn_open(NULL, 0); + if ( xe == NULL ) + do_err("xenevtchn_open() failed"); + + while ( true ) + { + t = xs_transaction_start(xs); + if ( t == XBT_NULL ) + do_err("xs_transaction_start() failed"); + + val = xs_read(xs, t, "/tool/xenlog/state", &len); + if ( val ) + { + free(val); + xs_transaction_end(xs, t, true); + do_err("daemon already running"); + } + + if ( !xs_write(xs, t, "/tool/xenlog/state", "running", + strlen("running")) ) + { + xs_transaction_end(xs, t, true); + do_err("xs_write() failed writing state"); + } + + if ( xs_transaction_end(xs, t, false) ) + break; + if ( errno != EAGAIN ) + do_err("xs_transaction_end() failed"); + } + + daemon_running = true; +} + +int main(int argc, char *argv[]) +{ + struct sigaction act = { .sa_handler = handle_stop, }; + int syslog_mask = LOG_MASK(LOG_WARNING) | LOG_MASK(LOG_ERR) | + LOG_MASK(LOG_CRIT) | LOG_MASK(LOG_ALERT) | + LOG_MASK(LOG_EMERG); + + umask(027); + if ( getenv("XENLOGD_VERBOSE") ) + syslog_mask |= LOG_MASK(LOG_NOTICE) | LOG_MASK(LOG_INFO); + openlog("xenlogd", LOG_CONS, LOG_DAEMON); + setlogmask(syslog_mask); + + sigemptyset(&act.sa_mask); + sigaction(SIGHUP, &act, NULL); + + xen_connect(); + + while ( !stop_me ) + { + sleep(60); + } + + close_all(); + + return 0; +}
Add "xenlogd", a new logging daemon meant to support infrastructure domains (e.g. xenstore-stubdom) to write log files in dom0. For now only add the code needed for starting the daemon and registering it with Xenstore via a new "/tool/xenlog/state" node by writing the "running" state to it. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/Makefile | 1 + tools/xenlogd/.gitignore | 1 + tools/xenlogd/Makefile | 38 ++++++++++ tools/xenlogd/xenlogd.c | 145 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 185 insertions(+) create mode 100644 tools/xenlogd/.gitignore create mode 100644 tools/xenlogd/Makefile create mode 100644 tools/xenlogd/xenlogd.c