diff mbox

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

Message ID 1468558636-17373-3-git-send-email-jgross@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Juergen Gross July 15, 2016, 4:57 a.m. UTC 
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(-)

Comments

Ian Jackson July 15, 2016, 4:21 p.m. UTC | #1 
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.
Juergen Gross July 18, 2016, 6:44 a.m. UTC | #2 
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
diff mbox

Patch

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);