All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] xenstore: fix memory leak of xenstored
@ 2016-07-15  4:57 Juergen Gross
  2016-07-15  4:57 ` [PATCH 1/2] xenstore: call each xenstored command function with temporary context Juergen Gross
  2016-07-15  4:57 ` [PATCH 2/2] xenstore: use temporary memory context for firing watches Juergen Gross
  0 siblings, 2 replies; 7+ messages in thread
From: Juergen Gross @ 2016-07-15  4:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson

xenstored has a memory leak when setting watches: a no longer active
watch which fired in the past will still use some memory. This is
critical for long running connections to xenstored like the qemu
process serving as a qdisk backend for dom0. It will use some few
kB in xenstored for each domain create/destroy pair.

Fix this leak by using a temporary memory context for all allocations
in xenstored when firing a watch event.

Juergen Gross (2):
  xenstore: call each xenstored command function with temporary context
  xenstore: use temporary memory context for firing watches

 tools/xenstore/xenstored_core.c        | 108 ++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |   4 ++
 tools/xenstore/xenstored_domain.c      |  20 +++---
 tools/xenstore/xenstored_domain.h      |  10 +--
 tools/xenstore/xenstored_transaction.c |   5 +-
 tools/xenstore/xenstored_transaction.h |   2 +-
 tools/xenstore/xenstored_watch.c       |  14 +++--
 tools/xenstore/xenstored_watch.h       |   3 +-
 8 files changed, 94 insertions(+), 72 deletions(-)

-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] xenstore: call each xenstored command function with temporary context
  2016-07-15  4:57 [PATCH 0/2] xenstore: fix memory leak of xenstored Juergen Gross
@ 2016-07-15  4:57 ` Juergen Gross
  2016-07-15 16:19   ` Ian Jackson
  2016-07-15  4:57 ` [PATCH 2/2] xenstore: use temporary memory context for firing watches Juergen Gross
  1 sibling, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2016-07-15  4:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson

In order to be able to avoid leaving temporary memory allocated after
processing of a command in xenstored call all command functions with
the temporary "in" context. Each function can then make use of that
temporary context for allocating temporary memory instead of either
leaving that memory allocated until the connection is dropped (or
even until end of xenstored) or freeing the memory itself.

This requires to modify the interfaces of the functions taking only
one argument from the connection.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c        | 39 +++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |  3 +++
 tools/xenstore/xenstored_domain.c      | 14 +++++++-----
 tools/xenstore/xenstored_domain.h      | 10 ++++-----
 tools/xenstore/xenstored_transaction.c |  3 ++-
 tools/xenstore/xenstored_transaction.h |  2 +-
 6 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 098865f..94c809c 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -713,7 +713,7 @@ bool is_valid_nodename(const char *node)
 /* We expect one arg in the input: return NULL otherwise.
  * The payload must contain exactly one nul, at the end.
  */
-static const char *onearg(struct buffered_data *in)
+const char *onearg(struct buffered_data *in)
 {
 	if (!in->used || get_string(in, 0) != in->used)
 		return NULL;
@@ -761,9 +761,10 @@ bool check_event_node(const char *node)
 	return true;
 }
 
-static void send_directory(struct connection *conn, const char *name)
+static void send_directory(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_READ);
@@ -775,9 +776,10 @@ static void send_directory(struct connection *conn, const char *name)
 	send_reply(conn, XS_DIRECTORY, node->children, node->childlen);
 }
 
-static void do_read(struct connection *conn, const char *name)
+static void do_read(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_READ);
@@ -943,9 +945,10 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	send_ack(conn, XS_WRITE);
 }
 
-static void do_mkdir(struct connection *conn, const char *name)
+static void do_mkdir(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_WRITE);
@@ -1060,9 +1063,10 @@ static void internal_rm(const char *name)
 }
 
 
-static void do_rm(struct connection *conn, const char *name)
+static void do_rm(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, name, XS_PERM_WRITE);
@@ -1094,9 +1098,10 @@ static void do_rm(struct connection *conn, const char *name)
 }
 
 
-static void do_get_perms(struct connection *conn, const char *name)
+static void do_get_perms(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	const char *name = onearg(in);
 	char *strings;
 	unsigned int len;
 
@@ -1210,11 +1215,11 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 
 	switch (in->hdr.msg.type) {
 	case XS_DIRECTORY:
-		send_directory(conn, onearg(in));
+		send_directory(conn, in);
 		break;
 
 	case XS_READ:
-		do_read(conn, onearg(in));
+		do_read(conn, in);
 		break;
 
 	case XS_WRITE:
@@ -1222,15 +1227,15 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_MKDIR:
-		do_mkdir(conn, onearg(in));
+		do_mkdir(conn, in);
 		break;
 
 	case XS_RM:
-		do_rm(conn, onearg(in));
+		do_rm(conn, in);
 		break;
 
 	case XS_GET_PERMS:
-		do_get_perms(conn, onearg(in));
+		do_get_perms(conn, in);
 		break;
 
 	case XS_SET_PERMS:
@@ -1254,7 +1259,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_TRANSACTION_END:
-		do_transaction_end(conn, onearg(in));
+		do_transaction_end(conn, in);
 		break;
 
 	case XS_INTRODUCE:
@@ -1262,19 +1267,19 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_IS_DOMAIN_INTRODUCED:
-		do_is_domain_introduced(conn, onearg(in));
+		do_is_domain_introduced(conn, in);
 		break;
 
 	case XS_RELEASE:
-		do_release(conn, onearg(in));
+		do_release(conn, in);
 		break;
 
 	case XS_GET_DOMAIN_PATH:
-		do_get_domain_path(conn, onearg(in));
+		do_get_domain_path(conn, in);
 		break;
 
 	case XS_RESUME:
-		do_resume(conn, onearg(in));
+		do_resume(conn, in);
 		break;
 
 	case XS_SET_TARGET:
@@ -1282,7 +1287,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		break;
 
 	case XS_RESET_WATCHES:
-		do_reset_watches(conn);
+		do_reset_watches(conn, in);
 		break;
 
 	default:
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3a497f7..5dbf9c8 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -122,6 +122,9 @@ struct node {
 	char *children;
 };
 
+/* Return the only argument in the input. */
+const char *onearg(struct buffered_data *in);
+
 /* Break input into vectors, return the number, fill in up to num of them. */
 unsigned int get_strings(struct buffered_data *data,
 			 char *vec[], unsigned int num);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 47b4f03..c66539a 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -453,8 +453,9 @@ void do_set_target(struct connection *conn, struct buffered_data *in)
 }
 
 /* domid */
-void do_release(struct connection *conn, const char *domid_str)
+void do_release(struct connection *conn, struct buffered_data *in)
 {
+	const char *domid_str = onearg(in);
 	struct domain *domain;
 	unsigned int domid;
 
@@ -490,10 +491,11 @@ void do_release(struct connection *conn, const char *domid_str)
 	send_ack(conn, XS_RELEASE);
 }
 
-void do_resume(struct connection *conn, const char *domid_str)
+void do_resume(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
 	unsigned int domid;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -527,9 +529,10 @@ void do_resume(struct connection *conn, const char *domid_str)
 	send_ack(conn, XS_RESUME);
 }
 
-void do_get_domain_path(struct connection *conn, const char *domid_str)
+void do_get_domain_path(struct connection *conn, struct buffered_data *in)
 {
 	char *path;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -543,10 +546,11 @@ void do_get_domain_path(struct connection *conn, const char *domid_str)
 	talloc_free(path);
 }
 
-void do_is_domain_introduced(struct connection *conn, const char *domid_str)
+void do_is_domain_introduced(struct connection *conn, struct buffered_data *in)
 {
 	int result;
 	unsigned int domid;
+	const char *domid_str = onearg(in);
 
 	if (!domid_str) {
 		send_error(conn, EINVAL);
@@ -563,7 +567,7 @@ void do_is_domain_introduced(struct connection *conn, const char *domid_str)
 }
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn)
+void do_reset_watches(struct connection *conn, struct buffered_data *in)
 {
 	conn_delete_all_watches(conn);
 	conn_delete_all_transactions(conn);
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 83488ed..2554423 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,22 +25,22 @@ void handle_event(void);
 void do_introduce(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_is_domain_introduced(struct connection *conn, const char *domid_str);
+void do_is_domain_introduced(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_release(struct connection *conn, const char *domid_str);
+void do_release(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_resume(struct connection *conn, const char *domid_str);
+void do_resume(struct connection *conn, struct buffered_data *in);
 
 /* domid, target */
 void do_set_target(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_get_domain_path(struct connection *conn, const char *domid_str);
+void do_get_domain_path(struct connection *conn, struct buffered_data *in);
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn);
+void do_reset_watches(struct connection *conn, struct buffered_data *in);
 
 void domain_init(void);
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index d0e4739..3cde26e 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -184,8 +184,9 @@ void do_transaction_start(struct connection *conn, struct buffered_data *in)
 	send_reply(conn, XS_TRANSACTION_START, id_str, strlen(id_str)+1);
 }
 
-void do_transaction_end(struct connection *conn, const char *arg)
+void do_transaction_end(struct connection *conn, struct buffered_data *in)
 {
+	const char *arg = onearg(in);
 	struct changed_node *i;
 	struct changed_domain *d;
 	struct transaction *trans;
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index cfeeae1..0c868ee 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -22,7 +22,7 @@
 struct transaction;
 
 void do_transaction_start(struct connection *conn, struct buffered_data *node);
-void do_transaction_end(struct connection *conn, const char *arg);
+void do_transaction_end(struct connection *conn, struct buffered_data *in);
 
 struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 
-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] xenstore: use temporary memory context for firing watches
  2016-07-15  4:57 [PATCH 0/2] xenstore: fix memory leak of xenstored Juergen Gross
  2016-07-15  4:57 ` [PATCH 1/2] xenstore: call each xenstored command function with temporary context Juergen Gross
@ 2016-07-15  4:57 ` Juergen Gross
  2016-07-15 16:21   ` Ian Jackson
  1 sibling, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2016-07-15  4:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson

Use a temporary memory context for memory allocations when firing
watches. This will avoid leaking memory in case of long living
connections and/or xenstore entries.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c        | 69 ++++++++++++++++++----------------
 tools/xenstore/xenstored_core.h        |  1 +
 tools/xenstore/xenstored_domain.c      |  6 +--
 tools/xenstore/xenstored_transaction.c |  2 +-
 tools/xenstore/xenstored_watch.c       | 14 ++++---
 tools/xenstore/xenstored_watch.h       |  3 +-
 6 files changed, 52 insertions(+), 43 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 94c809c..8cb12c7 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -398,7 +398,8 @@ bool is_child(const char *child, const char *parent)
 }
 
 /* If it fails, returns NULL and sets errno. */
-static struct node *read_node(struct connection *conn, const char *name)
+static struct node *read_node(struct connection *conn, const void *mem,
+			      const char *name)
 {
 	TDB_DATA key, data;
 	uint32_t *p;
@@ -419,7 +420,7 @@ static struct node *read_node(struct connection *conn, const char *name)
 		return NULL;
 	}
 
-	node = talloc(name, struct node);
+	node = talloc(mem, struct node);
 	node->name = talloc_strdup(node, name);
 	node->parent = NULL;
 	node->tdb = tdb_context(conn);
@@ -507,22 +508,23 @@ static enum xs_perm_type perm_for_conn(struct connection *conn,
 	return perms[0].perms & mask;
 }
 
-static char *get_parent(const char *node)
+static char *get_parent(const void *mem, const char *node)
 {
 	char *slash = strrchr(node + 1, '/');
 	if (!slash)
-		return talloc_strdup(node, "/");
-	return talloc_asprintf(node, "%.*s", (int)(slash - node), node);
+		return talloc_strdup(mem, "/");
+	return talloc_asprintf(mem, "%.*s", (int)(slash - node), node);
 }
 
 /* What do parents say? */
-static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
+static enum xs_perm_type ask_parents(struct connection *conn, const void *mem,
+				     const char *name)
 {
 	struct node *node;
 
 	do {
-		name = get_parent(name);
-		node = read_node(conn, name);
+		name = get_parent(mem, name);
+		node = read_node(conn, mem, name);
 		if (node)
 			break;
 	} while (!streq(name, "/"));
@@ -540,20 +542,22 @@ static enum xs_perm_type ask_parents(struct connection *conn, const char *name)
  * specific node without allowing it in the parents.  If it's going to
  * fail, however, we don't want the errno to indicate any information
  * about the node. */
-static int errno_from_parents(struct connection *conn, const char *node,
-			      int errnum, enum xs_perm_type perm)
+static int errno_from_parents(struct connection *conn, const void *mem,
+			      const char *node, int errnum,
+			      enum xs_perm_type perm)
 {
 	/* We always tell them about memory failures. */
 	if (errnum == ENOMEM)
 		return errnum;
 
-	if (ask_parents(conn, node) & perm)
+	if (ask_parents(conn, mem, node) & perm)
 		return errnum;
 	return EACCES;
 }
 
 /* If it fails, returns NULL and sets errno. */
 struct node *get_node(struct connection *conn,
+		      const void *mem,
 		      const char *name,
 		      enum xs_perm_type perm)
 {
@@ -563,7 +567,7 @@ struct node *get_node(struct connection *conn,
 		errno = EINVAL;
 		return NULL;
 	}
-	node = read_node(conn, name);
+	node = read_node(conn, mem, name);
 	/* If we don't have permission, we don't have node. */
 	if (node) {
 		if ((perm_for_conn(conn, node->perms, node->num_perms) & perm)
@@ -574,7 +578,7 @@ struct node *get_node(struct connection *conn,
 	}
 	/* Clean up errno if they weren't supposed to know. */
 	if (!node) 
-		errno = errno_from_parents(conn, name, errno, perm);
+		errno = errno_from_parents(conn, mem, name, errno, perm);
 	return node;
 }
 
@@ -767,7 +771,7 @@ static void send_directory(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -782,7 +786,7 @@ static void do_read(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -816,10 +820,10 @@ static struct node *construct_node(struct connection *conn, const char *name)
 	const char *base;
 	unsigned int baselen;
 	struct node *parent, *node;
-	char *children, *parentname = get_parent(name);
+	char *children, *parentname = get_parent(name, name);
 
 	/* If parent doesn't exist, create it. */
-	parent = read_node(conn, parentname);
+	parent = read_node(conn, parentname, parentname);
 	if (!parent)
 		parent = construct_node(conn, parentname);
 	if (!parent)
@@ -919,7 +923,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	datalen = in->used - offset;
 
 	name = canonicalize(conn, vec[0]);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 	if (!node) {
 		/* No permissions, invalid input? */
 		if (errno != ENOENT) {
@@ -941,7 +945,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	}
 
 	add_change_node(conn->transaction, name, false);
-	fire_watches(conn, name, false);
+	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_WRITE);
 }
 
@@ -951,7 +955,7 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 
 	/* If it already exists, fine. */
 	if (!node) {
@@ -966,7 +970,7 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 			return;
 		}
 		add_change_node(conn->transaction, name, false);
-		fire_watches(conn, name, false);
+		fire_watches(conn, in, name, false);
 	}
 	send_ack(conn, XS_MKDIR);
 }
@@ -984,7 +988,7 @@ static void delete_node(struct connection *conn, struct node *node)
 	for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) {
 		struct node *child;
 
-		child = read_node(conn, 
+		child = read_node(conn, node,
 				  talloc_asprintf(node, "%s/%s", node->name,
 						  node->children + i));
 		if (child) {
@@ -1036,7 +1040,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 	/* Delete from parent first, then if we crash, the worst that can
 	   happen is the child will continue to take up space, but will
 	   otherwise be unreachable. */
-	struct node *parent = read_node(conn, get_parent(name));
+	struct node *parent = read_node(conn, name, get_parent(name, name));
 	if (!parent) {
 		send_error(conn, EINVAL);
 		return 0;
@@ -1055,7 +1059,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 static void internal_rm(const char *name)
 {
 	char *tname = talloc_strdup(NULL, name);
-	struct node *node = read_node(NULL, tname);
+	struct node *node = read_node(NULL, tname, tname);
 	if (node)
 		_rm(NULL, node, tname);
 	talloc_free(node);
@@ -1069,11 +1073,11 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_WRITE);
+	node = get_node(conn, in, name, XS_PERM_WRITE);
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
-			node = read_node(conn, get_parent(name));
+			node = read_node(conn, in, get_parent(in, name));
 			if (node) {
 				send_ack(conn, XS_RM);
 				return;
@@ -1092,7 +1096,7 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 
 	if (_rm(conn, node, name)) {
 		add_change_node(conn->transaction, name, true);
-		fire_watches(conn, name, true);
+		fire_watches(conn, in, name, true);
 		send_ack(conn, XS_RM);
 	}
 }
@@ -1106,7 +1110,7 @@ static void do_get_perms(struct connection *conn, struct buffered_data *in)
 	unsigned int len;
 
 	name = canonicalize(conn, name);
-	node = get_node(conn, name, XS_PERM_READ);
+	node = get_node(conn, in, name, XS_PERM_READ);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -1138,7 +1142,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 	num--;
 
 	/* We must own node to do this (tools can do this too). */
-	node = get_node(conn, name, XS_PERM_WRITE|XS_PERM_OWNER);
+	node = get_node(conn, in, name, XS_PERM_WRITE|XS_PERM_OWNER);
 	if (!node) {
 		send_error(conn, errno);
 		return;
@@ -1168,7 +1172,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 	}
 
 	add_change_node(conn->transaction, name, false);
-	fire_watches(conn, name, false);
+	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_SET_PERMS);
 }
 
@@ -1604,7 +1608,7 @@ static void remember_string(struct hashtable *hash, const char *str)
  */
 static void check_store_(const char *name, struct hashtable *reachable)
 {
-	struct node *node = read_node(NULL, name);
+	struct node *node = read_node(NULL, name, name);
 
 	if (node) {
 		size_t i = 0;
@@ -1618,7 +1622,8 @@ static void check_store_(const char *name, struct hashtable *reachable)
 			size_t childlen = strlen(node->children + i);
 			char * childname = child_name(node->name,
 						      node->children + i);
-			struct node *childnode = read_node(NULL, childname);
+			struct node *childnode = read_node(NULL, childname,
+							   childname);
 			
 			if (childnode) {
 				if (hashtable_search(children, childname)) {
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 5dbf9c8..f763e47 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -149,6 +149,7 @@ bool check_event_node(const char *node);
 
 /* Get this node, checking we have permissions. */
 struct node *get_node(struct connection *conn,
+		      const void *mem,
 		      const char *name,
 		      enum xs_perm_type perm);
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index c66539a..5de93d4 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -204,7 +204,7 @@ static int destroy_domain(void *_domain)
 			unmap_interface(domain->interface);
 	}
 
-	fire_watches(NULL, "@releaseDomain", false);
+	fire_watches(NULL, domain, "@releaseDomain", false);
 
 	return 0;
 }
@@ -232,7 +232,7 @@ static void domain_cleanup(void)
 	}
 
 	if (notify)
-		fire_watches(NULL, "@releaseDomain", false);
+		fire_watches(NULL, NULL, "@releaseDomain", false);
 }
 
 /* We scan all domains rather than use the information given here. */
@@ -389,7 +389,7 @@ void do_introduce(struct connection *conn, struct buffered_data *in)
 		/* Now domain belongs to its connection. */
 		talloc_steal(domain->conn, domain);
 
-		fire_watches(NULL, "@introduceDomain", false);
+		fire_watches(NULL, in, "@introduceDomain", false);
 	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
 		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
 		if (domain->port)
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 3cde26e..34720fa 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -227,7 +227,7 @@ void do_transaction_end(struct connection *conn, struct buffered_data *in)
 
 		/* Fire off the watches for everything that changed. */
 		list_for_each_entry(i, &trans->changes, list)
-			fire_watches(conn, i->node, i->recurse);
+			fire_watches(conn, in, i->node, i->recurse);
 		generation++;
 	}
 	send_ack(conn, XS_TRANSACTION_END);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 8543999..8149701 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -48,6 +48,7 @@ struct watch
 };
 
 static void add_event(struct connection *conn,
+		      void *tmp,
 		      struct watch *watch,
 		      const char *name)
 {
@@ -57,7 +58,7 @@ static void add_event(struct connection *conn,
 
 	if (!check_event_node(name)) {
 		/* Can this conn load node, or see that it doesn't exist? */
-		struct node *node = get_node(conn, name, XS_PERM_READ);
+		struct node *node = get_node(conn, tmp, name, XS_PERM_READ);
 		/*
 		 * XXX We allow EACCES here because otherwise a non-dom0
 		 * backend driver cannot watch for disappearance of a frontend
@@ -78,14 +79,15 @@ static void add_event(struct connection *conn,
 	}
 
 	len = strlen(name) + 1 + strlen(watch->token) + 1;
-	data = talloc_array(watch, char, len);
+	data = talloc_array(tmp, char, len);
 	strcpy(data, name);
 	strcpy(data + strlen(name) + 1, watch->token);
 	send_reply(conn, XS_WATCH_EVENT, data, len);
 	talloc_free(data);
 }
 
-void fire_watches(struct connection *conn, const char *name, bool recurse)
+void fire_watches(struct connection *conn, void *tmp, const char *name,
+		  bool recurse)
 {
 	struct connection *i;
 	struct watch *watch;
@@ -98,9 +100,9 @@ void fire_watches(struct connection *conn, const char *name, bool recurse)
 	list_for_each_entry(i, &connections, list) {
 		list_for_each_entry(watch, &i->watches, list) {
 			if (is_child(name, watch->node))
-				add_event(i, watch, name);
+				add_event(i, tmp, watch, name);
 			else if (recurse && is_child(watch->node, name))
-				add_event(i, watch, watch->node);
+				add_event(i, tmp, watch, watch->node);
 		}
 	}
 }
@@ -169,7 +171,7 @@ void do_watch(struct connection *conn, struct buffered_data *in)
 	send_ack(conn, XS_WATCH);
 
 	/* We fire once up front: simplifies clients and restart. */
-	add_event(conn, watch, watch->node);
+	add_event(conn, in, watch, watch->node);
 }
 
 void do_unwatch(struct connection *conn, struct buffered_data *in)
diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h
index 5bc4f88..8ed1dde 100644
--- a/tools/xenstore/xenstored_watch.h
+++ b/tools/xenstore/xenstored_watch.h
@@ -25,7 +25,8 @@ void do_watch(struct connection *conn, struct buffered_data *in);
 void do_unwatch(struct connection *conn, struct buffered_data *in);
 
 /* Fire all watches: recurse means all the children are affected (ie. rm). */
-void fire_watches(struct connection *conn, const char *name, bool recurse);
+void fire_watches(struct connection *conn, void *tmp, const char *name,
+		  bool recurse);
 
 void dump_watches(struct connection *conn);
 
-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 1/2] xenstore: call each xenstored command function with temporary context
  2016-07-15  4:57 ` [PATCH 1/2] xenstore: call each xenstored command function with temporary context Juergen Gross
@ 2016-07-15 16:19   ` Ian Jackson
  2016-07-18  6:44     ` Juergen Gross
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2016-07-15 16:19 UTC (permalink / raw)
  To: Juergen Gross; +Cc: wei.liu2, xen-devel

Juergen Gross writes ("[PATCH 1/2] xenstore: call each xenstored command function with temporary context"):
> In order to be able to avoid leaving temporary memory allocated after
> processing of a command in xenstored call all command functions with
> the temporary "in" context. Each function can then make use of that
> temporary context for allocating temporary memory instead of either
> leaving that memory allocated until the connection is dropped (or
> even until end of xenstored) or freeing the memory itself.
> 
> This requires to modify the interfaces of the functions taking only
> one argument from the connection.

AFAICT this patch has no functional change, and is just the argument
changes to all these functions ?

If so please say that in the commit message!

If not, please split it out the functional change so that I can find
it...

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] xenstore: use temporary memory context for firing watches
  2016-07-15  4:57 ` [PATCH 2/2] xenstore: use temporary memory context for firing watches Juergen Gross
@ 2016-07-15 16:21   ` Ian Jackson
  2016-07-18  6:44     ` Juergen Gross
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2016-07-15 16:21 UTC (permalink / raw)
  To: Juergen Gross; +Cc: wei.liu2, xen-devel

Juergen Gross writes ("[PATCH 2/2] xenstore: use temporary memory context for firing watches"):
> Use a temporary memory context for memory allocations when firing
> watches. This will avoid leaking memory in case of long living
> connections and/or xenstore entries.

Can you please split out the non-functional argument change to
get_node and get_parent ?  This is very hard to review as-is.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] xenstore: call each xenstored command function with temporary context
  2016-07-15 16:19   ` Ian Jackson
@ 2016-07-18  6:44     ` Juergen Gross
  0 siblings, 0 replies; 7+ messages in thread
From: Juergen Gross @ 2016-07-18  6:44 UTC (permalink / raw)
  To: Ian Jackson; +Cc: wei.liu2, xen-devel

On 15/07/16 18:19, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 1/2] xenstore: call each xenstored command function with temporary context"):
>> In order to be able to avoid leaving temporary memory allocated after
>> processing of a command in xenstored call all command functions with
>> the temporary "in" context. Each function can then make use of that
>> temporary context for allocating temporary memory instead of either
>> leaving that memory allocated until the connection is dropped (or
>> even until end of xenstored) or freeing the memory itself.
>>
>> This requires to modify the interfaces of the functions taking only
>> one argument from the connection.
> 
> AFAICT this patch has no functional change, and is just the argument
> changes to all these functions ?
> 
> If so please say that in the commit message!

Okay.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] xenstore: use temporary memory context for firing watches
  2016-07-15 16:21   ` Ian Jackson
@ 2016-07-18  6:44     ` Juergen Gross
  0 siblings, 0 replies; 7+ messages in thread
From: Juergen Gross @ 2016-07-18  6:44 UTC (permalink / raw)
  To: Ian Jackson; +Cc: wei.liu2, xen-devel

On 15/07/16 18:21, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 2/2] xenstore: use temporary memory context for firing watches"):
>> Use a temporary memory context for memory allocations when firing
>> watches. This will avoid leaking memory in case of long living
>> connections and/or xenstore entries.
> 
> Can you please split out the non-functional argument change to
> get_node and get_parent ?  This is very hard to review as-is.

Okay.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-07-18  6:44 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-15  4:57 [PATCH 0/2] xenstore: fix memory leak of xenstored Juergen Gross
2016-07-15  4:57 ` [PATCH 1/2] xenstore: call each xenstored command function with temporary context Juergen Gross
2016-07-15 16:19   ` Ian Jackson
2016-07-18  6:44     ` Juergen Gross
2016-07-15  4:57 ` [PATCH 2/2] xenstore: use temporary memory context for firing watches Juergen Gross
2016-07-15 16:21   ` Ian Jackson
2016-07-18  6:44     ` Juergen Gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.