diff mbox series

[02/29] tools: add a new xen logging daemon

Message ID 20231101093325.30302-3-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools: enable xenstore-stubdom to use 9pfs | expand

Commit Message

Jürgen Groß Nov. 1, 2023, 9:32 a.m. UTC
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

Comments

Jason Andryuk Nov. 1, 2023, 6:36 p.m. UTC | #1
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
Jürgen Groß Nov. 2, 2023, 7:44 a.m. UTC | #2
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
Andrew Cooper Nov. 2, 2023, 7:15 p.m. UTC | #3
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
Jürgen Groß Nov. 3, 2023, 7:36 a.m. UTC | #4
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 mbox series

Patch

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;
+}