diff mbox series

[v3,14/16] tools/xenstore: split out environment specific live update code

Message ID 20230530085418.5417-15-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools/xenstore: more cleanups | expand

Commit Message

Juergen Gross May 30, 2023, 8:54 a.m. UTC
Instead of using #ifdef in xenstored_control.c split out the code of
environment specific functions (daemon or Mini-OS) to dedicated source
files.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/Makefile.common       |   8 +-
 tools/xenstore/xenstored_control.c   | 253 +--------------------------
 tools/xenstore/xenstored_lu.h        |  56 ++++++
 tools/xenstore/xenstored_lu_daemon.c | 133 ++++++++++++++
 tools/xenstore/xenstored_lu_minios.c | 121 +++++++++++++
 5 files changed, 317 insertions(+), 254 deletions(-)
 create mode 100644 tools/xenstore/xenstored_lu.h
 create mode 100644 tools/xenstore/xenstored_lu_daemon.c
 create mode 100644 tools/xenstore/xenstored_lu_minios.c

Comments

Julien Grall June 19, 2023, 5:55 p.m. UTC | #1
Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> -static struct live_update *lu_status;
> -
> -struct lu_dump_state {
> -	void *buf;
> -	unsigned int size;
> -#ifndef __MINIOS__
> -	int fd;
> -	char *filename;
> -#endif
> -};
> -
> -static int lu_destroy(void *data)
> -{
> -#ifdef __MINIOS__
> -	if (lu_status->dump_state)
> -		munmap(lu_status->dump_state, lu_status->dump_size);
> -#endif
> -	lu_status = NULL;
> -
> -	return 0;
> -}

I think moving all lu_destroy() out of xenstored_control.c is a mistake 
because we now need to remember that any common change in lu_begin() may 
need an update in the two implementation of lu_destroy().

Even if this seems pointless for a few lines, it would be best to split 
lu_destroy() in two parts: one common and one per lu backend.

The rest of the changes look good to me.

Cheers,
Juergen Gross June 20, 2023, 8:22 a.m. UTC | #2
On 19.06.23 19:55, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/05/2023 09:54, Juergen Gross wrote:
>> -static struct live_update *lu_status;
>> -
>> -struct lu_dump_state {
>> -    void *buf;
>> -    unsigned int size;
>> -#ifndef __MINIOS__
>> -    int fd;
>> -    char *filename;
>> -#endif
>> -};
>> -
>> -static int lu_destroy(void *data)
>> -{
>> -#ifdef __MINIOS__
>> -    if (lu_status->dump_state)
>> -        munmap(lu_status->dump_state, lu_status->dump_size);
>> -#endif
>> -    lu_status = NULL;
>> -
>> -    return 0;
>> -}
> 
> I think moving all lu_destroy() out of xenstored_control.c is a mistake because 
> we now need to remember that any common change in lu_begin() may need an update 
> in the two implementation of lu_destroy().
> 
> Even if this seems pointless for a few lines, it would be best to split 
> lu_destroy() in two parts: one common and one per lu backend.

Okay, fine with me.

> 
> The rest of the changes look good to me.

Thanks,

Juergen
diff mbox series

Patch

diff --git a/tools/xenstore/Makefile.common b/tools/xenstore/Makefile.common
index f71c9bfd55..c42796fe34 100644
--- a/tools/xenstore/Makefile.common
+++ b/tools/xenstore/Makefile.common
@@ -4,10 +4,10 @@  XENSTORED_OBJS-y := xenstored_core.o xenstored_watch.o xenstored_domain.o
 XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o
 XENSTORED_OBJS-y += talloc.o utils.o tdb.o hashtable.o
 
-XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_FreeBSD) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_MiniOS) += xenstored_minios.o
+XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_FreeBSD) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_MiniOS) += xenstored_minios.o xenstored_lu_minios.o
 
 # Include configure output (config.h)
 CFLAGS += -include $(XEN_ROOT)/tools/config.h
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 0d131e2ebc..b61b41f16c 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -38,63 +38,13 @@ 
 #include "xenstored_core.h"
 #include "xenstored_control.h"
 #include "xenstored_domain.h"
+#include "xenstored_lu.h"
 #include "xenstored_watch.h"
 
-/* Mini-OS only knows about MAP_ANON. */
-#ifndef MAP_ANONYMOUS
-#define MAP_ANONYMOUS MAP_ANON
-#endif
-
 #ifndef NO_LIVE_UPDATE
-struct live_update {
-	/* For verification the correct connection is acting. */
-	struct connection *conn;
-
-	/* Pointer to the command used to request LU */
-	struct buffered_data *in;
+struct live_update *lu_status;
 
-#ifdef __MINIOS__
-	void *kernel;
-	unsigned int kernel_size;
-	unsigned int kernel_off;
-
-	void *dump_state;
-	unsigned long dump_size;
-#else
-	char *filename;
-#endif
-
-	char *cmdline;
-
-	/* Start parameters. */
-	bool force;
-	unsigned int timeout;
-	time_t started_at;
-};
-
-static struct live_update *lu_status;
-
-struct lu_dump_state {
-	void *buf;
-	unsigned int size;
-#ifndef __MINIOS__
-	int fd;
-	char *filename;
-#endif
-};
-
-static int lu_destroy(void *data)
-{
-#ifdef __MINIOS__
-	if (lu_status->dump_state)
-		munmap(lu_status->dump_state, lu_status->dump_size);
-#endif
-	lu_status = NULL;
-
-	return 0;
-}
-
-static const char *lu_begin(struct connection *conn)
+const char *lu_begin(struct connection *conn)
 {
 	if (lu_status)
 		return "live-update session already active.";
@@ -431,203 +381,6 @@  static const char *lu_cmdline(const void *ctx, struct connection *conn,
 	return NULL;
 }
 
-#ifdef __MINIOS__
-static const char *lu_binary_alloc(const void *ctx, struct connection *conn,
-				   unsigned long size)
-{
-	const char *ret;
-
-	syslog(LOG_INFO, "live-update: binary size %lu\n", size);
-
-	ret = lu_begin(conn);
-	if (ret)
-		return ret;
-
-	lu_status->kernel = talloc_size(lu_status, size);
-	if (!lu_status->kernel)
-		return "Allocation failure.";
-
-	lu_status->kernel_size = size;
-	lu_status->kernel_off = 0;
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_binary_save(const void *ctx, struct connection *conn,
-				  unsigned int size, const char *data)
-{
-	if (!lu_status || lu_status->conn != conn)
-		return "Not in live-update session.";
-
-	if (lu_status->kernel_off + size > lu_status->kernel_size)
-		return "Too much kernel data.";
-
-	memcpy(lu_status->kernel + lu_status->kernel_off, data, size);
-	lu_status->kernel_off += size;
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_arch(const void *ctx, struct connection *conn,
-			   char **vec, int num)
-{
-	if (num == 2 && !strcmp(vec[0], "-b"))
-		return lu_binary_alloc(ctx, conn, atol(vec[1]));
-	if (num > 2 && !strcmp(vec[0], "-d"))
-		return lu_binary_save(ctx, conn, atoi(vec[1]), vec[2]);
-
-	errno = EINVAL;
-	return NULL;
-}
-
-static FILE *lu_dump_open(const void *ctx)
-{
-	lu_status->dump_size = ROUNDUP(talloc_total_size(NULL) * 2,
-				       XC_PAGE_SHIFT);
-	lu_status->dump_state = mmap(NULL, lu_status->dump_size,
-				     PROT_READ | PROT_WRITE,
-				     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-	if (lu_status->dump_state == MAP_FAILED)
-		return NULL;
-
-	return fmemopen(lu_status->dump_state, lu_status->dump_size, "w");
-}
-
-static void lu_dump_close(FILE *fp)
-{
-	size_t size;
-
-	size = ftell(fp);
-	size = ROUNDUP(size, XC_PAGE_SHIFT);
-	munmap(lu_status->dump_state + size, lu_status->dump_size - size);
-	lu_status->dump_size = size;
-
-	fclose(fp);
-}
-
-static void lu_get_dump_state(struct lu_dump_state *state)
-{
-}
-
-static void lu_close_dump_state(struct lu_dump_state *state)
-{
-}
-
-static char *lu_exec(const void *ctx, int argc, char **argv)
-{
-	return "NYI";
-}
-#else
-static const char *lu_binary(const void *ctx, struct connection *conn,
-			     const char *filename)
-{
-	const char *ret;
-	struct stat statbuf;
-
-	syslog(LOG_INFO, "live-update: binary %s\n", filename);
-
-	if (stat(filename, &statbuf))
-		return "File not accessible.";
-	if (!(statbuf.st_mode & (S_IXOTH | S_IXGRP | S_IXUSR)))
-		return "File not executable.";
-
-	ret = lu_begin(conn);
-	if (ret)
-		return ret;
-
-	lu_status->filename = talloc_strdup(lu_status, filename);
-	if (!lu_status->filename)
-		return "Allocation failure.";
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_arch(const void *ctx, struct connection *conn,
-			   char **vec, int num)
-{
-	if (num == 2 && !strcmp(vec[0], "-f"))
-		return lu_binary(ctx, conn, vec[1]);
-
-	errno = EINVAL;
-	return NULL;
-}
-
-static FILE *lu_dump_open(const void *ctx)
-{
-	char *filename;
-	int fd;
-
-	filename = talloc_asprintf(ctx, "%s/state_dump",
-				   xenstore_daemon_rundir());
-	if (!filename)
-		return NULL;
-
-	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
-	if (fd < 0)
-		return NULL;
-
-	return fdopen(fd, "w");
-}
-
-static void lu_dump_close(FILE *fp)
-{
-	fclose(fp);
-}
-
-static void lu_get_dump_state(struct lu_dump_state *state)
-{
-	struct stat statbuf;
-
-	state->size = 0;
-
-	state->filename = talloc_asprintf(NULL, "%s/state_dump",
-					  xenstore_daemon_rundir());
-	if (!state->filename)
-		barf("Allocation failure");
-
-	state->fd = open(state->filename, O_RDONLY);
-	if (state->fd < 0)
-		return;
-	if (fstat(state->fd, &statbuf) != 0)
-		goto out_close;
-	state->size = statbuf.st_size;
-
-	state->buf = mmap(NULL, state->size, PROT_READ, MAP_PRIVATE,
-			  state->fd, 0);
-	if (state->buf == MAP_FAILED) {
-		state->size = 0;
-		goto out_close;
-	}
-
-	return;
-
- out_close:
-	close(state->fd);
-}
-
-static void lu_close_dump_state(struct lu_dump_state *state)
-{
-	assert(state->filename != NULL);
-
-	munmap(state->buf, state->size);
-	close(state->fd);
-
-	unlink(state->filename);
-	talloc_free(state->filename);
-}
-
-static char *lu_exec(const void *ctx, int argc, char **argv)
-{
-	argv[0] = lu_status->filename;
-	execvp(argv[0], argv);
-
-	return "Error activating new binary.";
-}
-#endif
-
 static bool lu_check_lu_allowed(void)
 {
 	struct connection *conn;
diff --git a/tools/xenstore/xenstored_lu.h b/tools/xenstore/xenstored_lu.h
new file mode 100644
index 0000000000..d2f8e4e57c
--- /dev/null
+++ b/tools/xenstore/xenstored_lu.h
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Live Update interfaces for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#ifndef NO_LIVE_UPDATE
+struct live_update {
+	/* For verification the correct connection is acting. */
+	struct connection *conn;
+
+	/* Pointer to the command used to request LU */
+	struct buffered_data *in;
+
+#ifdef __MINIOS__
+	void *kernel;
+	unsigned int kernel_size;
+	unsigned int kernel_off;
+
+	void *dump_state;
+	unsigned long dump_size;
+#else
+	char *filename;
+#endif
+
+	char *cmdline;
+
+	/* Start parameters. */
+	bool force;
+	unsigned int timeout;
+	time_t started_at;
+};
+
+struct lu_dump_state {
+	void *buf;
+	unsigned int size;
+#ifndef __MINIOS__
+	int fd;
+	char *filename;
+#endif
+};
+
+extern struct live_update *lu_status;
+
+/* Live update private interfaces. */
+void lu_get_dump_state(struct lu_dump_state *state);
+void lu_close_dump_state(struct lu_dump_state *state);
+FILE *lu_dump_open(const void *ctx);
+void lu_dump_close(FILE *fp);
+char *lu_exec(const void *ctx, int argc, char **argv);
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num);
+const char *lu_begin(struct connection *conn);
+int lu_destroy(void *data);
+#endif
diff --git a/tools/xenstore/xenstored_lu_daemon.c b/tools/xenstore/xenstored_lu_daemon.c
new file mode 100644
index 0000000000..952d9f0b25
--- /dev/null
+++ b/tools/xenstore/xenstored_lu_daemon.c
@@ -0,0 +1,133 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * Live Update for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <syslog.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <xen-tools/xenstore-common.h>
+
+#include "talloc.h"
+#include "xenstored_core.h"
+#include "xenstored_lu.h"
+
+#ifndef NO_LIVE_UPDATE
+void lu_get_dump_state(struct lu_dump_state *state)
+{
+	struct stat statbuf;
+
+	state->size = 0;
+
+	state->filename = talloc_asprintf(NULL, "%s/state_dump",
+					  xenstore_daemon_rundir());
+	if (!state->filename)
+		barf("Allocation failure");
+
+	state->fd = open(state->filename, O_RDONLY);
+	if (state->fd < 0)
+		return;
+	if (fstat(state->fd, &statbuf) != 0)
+		goto out_close;
+	state->size = statbuf.st_size;
+
+	state->buf = mmap(NULL, state->size, PROT_READ, MAP_PRIVATE,
+			  state->fd, 0);
+	if (state->buf == MAP_FAILED) {
+		state->size = 0;
+		goto out_close;
+	}
+
+	return;
+
+ out_close:
+	close(state->fd);
+}
+
+void lu_close_dump_state(struct lu_dump_state *state)
+{
+	assert(state->filename != NULL);
+
+	munmap(state->buf, state->size);
+	close(state->fd);
+
+	unlink(state->filename);
+	talloc_free(state->filename);
+}
+
+FILE *lu_dump_open(const void *ctx)
+{
+	char *filename;
+	int fd;
+
+	filename = talloc_asprintf(ctx, "%s/state_dump",
+				   xenstore_daemon_rundir());
+	if (!filename)
+		return NULL;
+
+	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
+	if (fd < 0)
+		return NULL;
+
+	return fdopen(fd, "w");
+}
+
+void lu_dump_close(FILE *fp)
+{
+	fclose(fp);
+}
+
+char *lu_exec(const void *ctx, int argc, char **argv)
+{
+	argv[0] = lu_status->filename;
+	execvp(argv[0], argv);
+
+	return "Error activating new binary.";
+}
+
+int lu_destroy(void *data)
+{
+	lu_status = NULL;
+
+	return 0;
+}
+
+static const char *lu_binary(const void *ctx, struct connection *conn,
+			     const char *filename)
+{
+	const char *ret;
+	struct stat statbuf;
+
+	syslog(LOG_INFO, "live-update: binary %s\n", filename);
+
+	if (stat(filename, &statbuf))
+		return "File not accessible.";
+	if (!(statbuf.st_mode & (S_IXOTH | S_IXGRP | S_IXUSR)))
+		return "File not executable.";
+
+	ret = lu_begin(conn);
+	if (ret)
+		return ret;
+
+	lu_status->filename = talloc_strdup(lu_status, filename);
+	if (!lu_status->filename)
+		return "Allocation failure.";
+
+	errno = 0;
+	return NULL;
+}
+
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num)
+{
+	if (num == 2 && !strcmp(vec[0], "-f"))
+		return lu_binary(ctx, conn, vec[1]);
+
+	errno = EINVAL;
+	return NULL;
+}
+#endif
diff --git a/tools/xenstore/xenstored_lu_minios.c b/tools/xenstore/xenstored_lu_minios.c
new file mode 100644
index 0000000000..0bec8a0037
--- /dev/null
+++ b/tools/xenstore/xenstored_lu_minios.c
@@ -0,0 +1,121 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * Live Update for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <syslog.h>
+#include <sys/mman.h>
+#include <xenctrl.h>
+#include <xen-tools/common-macros.h>
+
+#include "talloc.h"
+#include "xenstored_lu.h"
+
+/* Mini-OS only knows about MAP_ANON. */
+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+
+#ifndef NO_LIVE_UPDATE
+void lu_get_dump_state(struct lu_dump_state *state)
+{
+}
+
+void lu_close_dump_state(struct lu_dump_state *state)
+{
+}
+
+FILE *lu_dump_open(const void *ctx)
+{
+	lu_status->dump_size = ROUNDUP(talloc_total_size(NULL) * 2,
+				       XC_PAGE_SHIFT);
+	lu_status->dump_state = mmap(NULL, lu_status->dump_size,
+				     PROT_READ | PROT_WRITE,
+				     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (lu_status->dump_state == MAP_FAILED)
+		return NULL;
+
+	return fmemopen(lu_status->dump_state, lu_status->dump_size, "w");
+}
+
+void lu_dump_close(FILE *fp)
+{
+	size_t size;
+
+	size = ftell(fp);
+	size = ROUNDUP(size, XC_PAGE_SHIFT);
+	munmap(lu_status->dump_state + size, lu_status->dump_size - size);
+	lu_status->dump_size = size;
+
+	fclose(fp);
+}
+
+char *lu_exec(const void *ctx, int argc, char **argv)
+{
+	return "NYI";
+}
+
+int lu_destroy(void *data)
+{
+	if (lu_status->dump_state)
+		munmap(lu_status->dump_state, lu_status->dump_size);
+	lu_status = NULL;
+
+	return 0;
+}
+
+static const char *lu_binary_alloc(const void *ctx, struct connection *conn,
+				   unsigned long size)
+{
+	const char *ret;
+
+	syslog(LOG_INFO, "live-update: binary size %lu\n", size);
+
+	ret = lu_begin(conn);
+	if (ret)
+		return ret;
+
+	lu_status->kernel = talloc_size(lu_status, size);
+	if (!lu_status->kernel)
+		return "Allocation failure.";
+
+	lu_status->kernel_size = size;
+	lu_status->kernel_off = 0;
+
+	errno = 0;
+	return NULL;
+}
+
+static const char *lu_binary_save(const void *ctx, struct connection *conn,
+				  unsigned int size, const char *data)
+{
+	if (!lu_status || lu_status->conn != conn)
+		return "Not in live-update session.";
+
+	if (lu_status->kernel_off + size > lu_status->kernel_size)
+		return "Too much kernel data.";
+
+	memcpy(lu_status->kernel + lu_status->kernel_off, data, size);
+	lu_status->kernel_off += size;
+
+	errno = 0;
+	return NULL;
+}
+
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num)
+{
+	if (num == 2 && !strcmp(vec[0], "-b"))
+		return lu_binary_alloc(ctx, conn, atol(vec[1]));
+	if (num > 2 && !strcmp(vec[0], "-d"))
+		return lu_binary_save(ctx, conn, atoi(vec[1]), vec[2]);
+
+	errno = EINVAL;
+	return NULL;
+}
+#endif