[PATCH 0/2] xenstore: fix memory leak of xenstored

[PATCH 0/2] xenstore: fix memory leak of xenstored

[Juergen Gross]

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

[PATCH 1/2] xenstore: call each xenstored command function with temporary context

[Juergen Gross]

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

[PATCH 1/2] xenstore: call each xenstored command function with temporary context

[Ian Jackson]

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

Re: [PATCH 1/2] xenstore: call each xenstored command function with temporary context

[Juergen Gross]

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

[PATCH 2/2] xenstore: use temporary memory context for firing watches

[Juergen Gross]

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

[PATCH 2/2] xenstore: use temporary memory context for firing watches

[Ian Jackson]

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

Re: [PATCH 2/2] xenstore: use temporary memory context for firing watches

[Juergen Gross]

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