[PATCH v4 00/12] xenstore: support reading directory with many children

[PATCH v4 00/12] xenstore: support reading directory with many children

[Juergen Gross]

Reading the children list of a xenstore node with the length of that
list exceeding 4096 bytes is currently not possible. This can be a
problem for a large host with a huge number of domains as Xen tools
will no longer by capable to scan some directories of xenstore (e.g.
/local/domain).

This patch series adds a new xs wire command to read a directory
in multiple chunks. libxenstore is modified in a compatible way to
show an unmodified result in case xenstored doesn't support the new
command.

As there have been questions regarding handling of memory allocation
failures I've decided to add proper handling of those, requiring some
more rework.

The patch set has been verified to work by using the following shell script:

xenstore-write /test "test"

for i in `seq 100 500`
do
    xenstore-write /test/entry_with_very_long_name_$i $i
done

xenstore-ls
xenstore-rm /test

Xenstore has been verified to work by starting multiple domain types.
Especially HVM with qemu-stubdom has been tested as this configuration
seems to be rather sensible to concurrent transactions.

Changes in V4:
- move introduction of XS_NEXT_ENTRY from patch 5 to patch 7 and rename it
  to XS_TYPE_COUNT as rewuested by Jan Beulich
- patch 9: don't remove functions from libxenstore as requested by Andrew
  Cooper
- patch 12: add some more allocation failure checks

Changes in V3:
- remove patch 1, as it has been applied already
- new patches 7-12
- some minor modifications in patch 5 (was 6) as suggested by Jan Beulich

Changes in V2:
- complete rework as suggested by Jan Beulich: don't use transactions
  for consistency, but a per-node generation count
- fix a (minor?) problem in transaction code regarding watches (patch 1)

Juergen Gross (12):
  xenstore: modify add_change_node() parameter types
  xenstore: call add_change_node() directly when writing node
  xenstore: use common tdb record header in xenstore
  xenstore: add per-node generation counter
  xenstore: add support for reading directory with many children
  xenstore: support XS_DIRECTORY_PART in libxenstore
  xenstore: use array for xenstore wire command handling
  xenstore: let command functions return error or success
  xenstore: make functions static
  xenstore: add helper functions for wire argument parsing
  xenstore: add small default data buffer to internal struct
  xenstore: handle memory allocation failures in xenstored

 tools/xenstore/include/xenstore_lib.h  |   9 +
 tools/xenstore/xenstored_core.c        | 824 ++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |  23 +-
 tools/xenstore/xenstored_domain.c      | 218 ++++-----
 tools/xenstore/xenstored_domain.h      |  14 +-
 tools/xenstore/xenstored_transaction.c | 106 +++--
 tools/xenstore/xenstored_transaction.h |   8 +-
 tools/xenstore/xenstored_watch.c       |  89 ++--
 tools/xenstore/xenstored_watch.h       |   6 +-
 tools/xenstore/xs.c                    | 191 +++++++-
 tools/xenstore/xs_lib.c                | 112 -----
 tools/xenstore/xs_tdb_dump.c           |  11 +-
 xen/include/public/io/xs_wire.h        |   3 +
 13 files changed, 890 insertions(+), 724 deletions(-)

-- 
2.10.2


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

[PATCH v4 01/12] xenstore: modify add_change_node() parameter types

[Juergen Gross]

In order to prepare adding a generation count to each node modify
add_change_node() to take the connection pointer and a node pointer
instead of the transaction pointer and node name as parameters. This
requires moving the call of add_change_node() from do_rm() to
delete_node_single().

While at it correct the comment for the prototype: there is no
longjmp() involved.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c        | 23 ++++++++++++++---------
 tools/xenstore/xenstored_transaction.c | 11 +++++++----
 tools/xenstore/xenstored_transaction.h |  4 ++--
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3df977b..de1a9b4 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -822,7 +822,8 @@ static void do_read(struct connection *conn, struct buffered_data *in)
 	send_reply(conn, XS_READ, node->data, node->datalen);
 }
 
-static void delete_node_single(struct connection *conn, struct node *node)
+static void delete_node_single(struct connection *conn, struct node *node,
+			       bool changed)
 {
 	TDB_DATA key;
 
@@ -833,6 +834,10 @@ static void delete_node_single(struct connection *conn, struct node *node)
 		corrupt(conn, "Could not delete '%s'", node->name);
 		return;
 	}
+
+	if (changed)
+		add_change_node(conn, node, true);
+
 	domain_entry_dec(conn, node);
 }
 
@@ -971,7 +976,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 		}
 	}
 
-	add_change_node(conn->transaction, name, false);
+	add_change_node(conn, node, false);
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_WRITE);
 }
@@ -1002,20 +1007,21 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 			send_error(conn, errno);
 			return;
 		}
-		add_change_node(conn->transaction, name, false);
+		add_change_node(conn, node, false);
 		fire_watches(conn, in, name, false);
 	}
 	send_ack(conn, XS_MKDIR);
 }
 
-static void delete_node(struct connection *conn, struct node *node)
+static void delete_node(struct connection *conn, struct node *node,
+			bool changed)
 {
 	unsigned int i;
 
 	/* Delete self, then delete children.  If we crash, then the worst
 	   that can happen is the children will continue to take up space, but
 	   will otherwise be unreachable. */
-	delete_node_single(conn, node);
+	delete_node_single(conn, node, changed);
 
 	/* Delete children, too. */
 	for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) {
@@ -1025,7 +1031,7 @@ static void delete_node(struct connection *conn, struct node *node)
 				  talloc_asprintf(node, "%s/%s", node->name,
 						  node->children + i));
 		if (child) {
-			delete_node(conn, child);
+			delete_node(conn, child, false);
 		}
 		else {
 			trace("delete_node: No child '%s/%s' found!\n",
@@ -1084,7 +1090,7 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 		return 0;
 	}
 
-	delete_node(conn, node);
+	delete_node(conn, node, true);
 	return 1;
 }
 
@@ -1128,7 +1134,6 @@ 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, in, name, true);
 		send_ack(conn, XS_RM);
 	}
@@ -1204,7 +1209,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 		return;
 	}
 
-	add_change_node(conn->transaction, name, false);
+	add_change_node(conn, node, false);
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_SET_PERMS);
 }
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 84cb0bf..b08b2eb 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -92,18 +92,21 @@ TDB_CONTEXT *tdb_transaction_context(struct transaction *trans)
 
 /* Callers get a change node (which can fail) and only commit after they've
  * finished.  This way they don't have to unwind eg. a write. */
-void add_change_node(struct transaction *trans, const char *node, bool recurse)
+void add_change_node(struct connection *conn, struct node *node, bool recurse)
 {
 	struct changed_node *i;
+	struct transaction *trans;
 
-	if (!trans) {
+	if (!conn || !conn->transaction) {
 		/* They're changing the global database. */
 		generation++;
 		return;
 	}
 
+	trans = conn->transaction;
+
 	list_for_each_entry(i, &trans->changes, list) {
-		if (streq(i->node, node)) {
+		if (streq(i->node, node->name)) {
 			if (recurse)
 				i->recurse = recurse;
 			return;
@@ -111,7 +114,7 @@ void add_change_node(struct transaction *trans, const char *node, bool recurse)
 	}
 
 	i = talloc(trans, struct changed_node);
-	i->node = talloc_strdup(i, node);
+	i->node = talloc_strdup(i, node->name);
 	i->recurse = recurse;
 	list_add_tail(&i->list, &trans->changes);
 }
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index 0c868ee..7f0a781 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -30,8 +30,8 @@ struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 void transaction_entry_inc(struct transaction *trans, unsigned int domid);
 void transaction_entry_dec(struct transaction *trans, unsigned int domid);
 
-/* This node was changed: can fail and longjmp. */
-void add_change_node(struct transaction *trans, const char *node,
+/* This node was changed. */
+void add_change_node(struct connection *conn, struct node *node,
                      bool recurse);
 
 /* Return tdb context to use for this connection. */
-- 
2.10.2


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

[PATCH v4 02/12] xenstore: call add_change_node() directly when writing node

[Juergen Gross]

Instead of calling add_change_node() at places where write_node() is
called, do that inside write_node().

Note that there is one case where add_change_node() is called now when
a later failure will prohibit the changed node to be written: in case
of a write_node failing due to an error in tdb_store(). As the only
visible change of behavior is a stale event fired for the node, while
the failing tdb_store() signals a corrupted xenstore database, the
stale event will be the least problem of this case.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index de1a9b4..1354387 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -456,7 +456,7 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 	return node;
 }
 
-static bool write_node(struct connection *conn, const struct node *node)
+static bool write_node(struct connection *conn, struct node *node)
 {
 	/*
 	 * conn will be null when this is called from manual_node.
@@ -476,6 +476,8 @@ static bool write_node(struct connection *conn, const struct node *node)
 	if (domain_is_unprivileged(conn) && data.dsize >= quota_max_entry_size)
 		goto error;
 
+	add_change_node(conn, node, false);
+
 	data.dptr = talloc_size(node, data.dsize);
 	((uint32_t *)data.dptr)[0] = node->num_perms;
 	((uint32_t *)data.dptr)[1] = node->datalen;
@@ -976,7 +978,6 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 		}
 	}
 
-	add_change_node(conn, node, false);
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_WRITE);
 }
@@ -1007,7 +1008,6 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 			send_error(conn, errno);
 			return;
 		}
-		add_change_node(conn, node, false);
 		fire_watches(conn, in, name, false);
 	}
 	send_ack(conn, XS_MKDIR);
@@ -1209,7 +1209,6 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 		return;
 	}
 
-	add_change_node(conn, node, false);
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_SET_PERMS);
 }
-- 
2.10.2


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

Re: [PATCH v4 02/12] xenstore: call add_change_node() directly when writing node

[Wei Liu]

On Mon, Dec 05, 2016 at 08:48:43AM +0100, Juergen Gross wrote:
> Instead of calling add_change_node() at places where write_node() is
> called, do that inside write_node().
> 
> Note that there is one case where add_change_node() is called now when
> a later failure will prohibit the changed node to be written: in case
> of a write_node failing due to an error in tdb_store(). As the only
> visible change of behavior is a stale event fired for the node, while
> the failing tdb_store() signals a corrupted xenstore database, the
> stale event will be the least problem of this case.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v4 03/12] xenstore: use common tdb record header in xenstore

[Juergen Gross]

The layout of the tdb record of xenstored is defined at multiple
places: read_node(), write_node() and in xs_tdb_dump.c

Use a common structure instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/include/xenstore_lib.h |  8 ++++++++
 tools/xenstore/xenstored_core.c       | 27 ++++++++++++++-------------
 tools/xenstore/xs_tdb_dump.c          | 11 ++---------
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h
index 462b7b9..efdf935 100644
--- a/tools/xenstore/include/xenstore_lib.h
+++ b/tools/xenstore/include/xenstore_lib.h
@@ -42,6 +42,14 @@ struct xs_permissions
 	enum xs_perm_type perms;
 };
 
+/* Header of the node record in tdb. */
+struct xs_tdb_record_hdr {
+	uint32_t num_perms;
+	uint32_t datalen;
+	uint32_t childlen;
+	struct xs_permissions perms[0];
+};
+
 /* Each 10 bits takes ~ 3 digits, plus one, plus one for nul terminator. */
 #define MAX_STRLEN(x) ((sizeof(x) * CHAR_BIT + CHAR_BIT-1) / 10 * 3 + 2)
 
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1354387..dfad0d5 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -416,7 +416,7 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 			      const char *name)
 {
 	TDB_DATA key, data;
-	uint32_t *p;
+	struct xs_tdb_record_hdr *hdr;
 	struct node *node;
 	TDB_CONTEXT * context = tdb_context(conn);
 
@@ -441,13 +441,13 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 	talloc_steal(node, data.dptr);
 
 	/* Datalen, childlen, number of permissions */
-	p = (uint32_t *)data.dptr;
-	node->num_perms = p[0];
-	node->datalen = p[1];
-	node->childlen = p[2];
+	hdr = (void *)data.dptr;
+	node->num_perms = hdr->num_perms;
+	node->datalen = hdr->datalen;
+	node->childlen = hdr->childlen;
 
 	/* Permissions are struct xs_permissions. */
-	node->perms = (void *)&p[3];
+	node->perms = hdr->perms;
 	/* Data is binary blob (usually ascii, no nul). */
 	node->data = node->perms + node->num_perms;
 	/* Children is strings, nul separated. */
@@ -465,11 +465,12 @@ static bool write_node(struct connection *conn, struct node *node)
 
 	TDB_DATA key, data;
 	void *p;
+	struct xs_tdb_record_hdr *hdr;
 
 	key.dptr = (void *)node->name;
 	key.dsize = strlen(node->name);
 
-	data.dsize = 3*sizeof(uint32_t)
+	data.dsize = sizeof(*hdr)
 		+ node->num_perms*sizeof(node->perms[0])
 		+ node->datalen + node->childlen;
 
@@ -479,13 +480,13 @@ static bool write_node(struct connection *conn, struct node *node)
 	add_change_node(conn, node, false);
 
 	data.dptr = talloc_size(node, data.dsize);
-	((uint32_t *)data.dptr)[0] = node->num_perms;
-	((uint32_t *)data.dptr)[1] = node->datalen;
-	((uint32_t *)data.dptr)[2] = node->childlen;
-	p = data.dptr + 3 * sizeof(uint32_t);
+	hdr = (void *)data.dptr;
+	hdr->num_perms = node->num_perms;
+	hdr->datalen = node->datalen;
+	hdr->childlen = node->childlen;
 
-	memcpy(p, node->perms, node->num_perms*sizeof(node->perms[0]));
-	p += node->num_perms*sizeof(node->perms[0]);
+	memcpy(hdr->perms, node->perms, node->num_perms*sizeof(node->perms[0]));
+	p = hdr->perms + node->num_perms;
 	memcpy(p, node->data, node->datalen);
 	p += node->datalen;
 	memcpy(p, node->children, node->childlen);
diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c
index 9f636f9..207ed44 100644
--- a/tools/xenstore/xs_tdb_dump.c
+++ b/tools/xenstore/xs_tdb_dump.c
@@ -11,14 +11,7 @@
 #include "talloc.h"
 #include "utils.h"
 
-struct record_hdr {
-	uint32_t num_perms;
-	uint32_t datalen;
-	uint32_t childlen;
-	struct xs_permissions perms[0];
-};
-
-static uint32_t total_size(struct record_hdr *hdr)
+static uint32_t total_size(struct xs_tdb_record_hdr *hdr)
 {
 	return sizeof(*hdr) + hdr->num_perms * sizeof(struct xs_permissions) 
 		+ hdr->datalen + hdr->childlen;
@@ -58,7 +51,7 @@ int main(int argc, char *argv[])
 	key = tdb_firstkey(tdb);
 	while (key.dptr) {
 		TDB_DATA data;
-		struct record_hdr *hdr;
+		struct xs_tdb_record_hdr *hdr;
 
 		data = tdb_fetch(tdb, key);
 		hdr = (void *)data.dptr;
-- 
2.10.2


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

[PATCH v4 04/12] xenstore: add per-node generation counter

[Juergen Gross]

In order to be able to support reading the list of a node's children in
multiple chunks (needed for list sizes > 4096 bytes) without having to
allocate a temporary buffer we need some kind of generation counter for
each node. This will help to recognize a node has changed between
reading two chunks.

As removing a node and reintroducing it must result in different
generation counts each generation value has to be globally unique. This
can be ensured only by using a global 64 bit counter.

For handling of transactions there is already such a counter available,
it just has to be expanded to 64 bits and must be stored in each
modified node.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/include/xenstore_lib.h  |  1 +
 tools/xenstore/xenstored_core.c        |  2 ++
 tools/xenstore/xenstored_core.h        |  3 +++
 tools/xenstore/xenstored_transaction.c | 15 ++++++++++-----
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h
index efdf935..0ffbae9 100644
--- a/tools/xenstore/include/xenstore_lib.h
+++ b/tools/xenstore/include/xenstore_lib.h
@@ -44,6 +44,7 @@ struct xs_permissions
 
 /* Header of the node record in tdb. */
 struct xs_tdb_record_hdr {
+	uint64_t generation;
 	uint32_t num_perms;
 	uint32_t datalen;
 	uint32_t childlen;
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index dfad0d5..95d6d7d 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -442,6 +442,7 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 
 	/* Datalen, childlen, number of permissions */
 	hdr = (void *)data.dptr;
+	node->generation = hdr->generation;
 	node->num_perms = hdr->num_perms;
 	node->datalen = hdr->datalen;
 	node->childlen = hdr->childlen;
@@ -481,6 +482,7 @@ static bool write_node(struct connection *conn, struct node *node)
 
 	data.dptr = talloc_size(node, data.dsize);
 	hdr = (void *)data.dptr;
+	hdr->generation = node->generation;
 	hdr->num_perms = node->num_perms;
 	hdr->datalen = node->datalen;
 	hdr->childlen = node->childlen;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index ecc614f..089625f 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -109,6 +109,9 @@ struct node {
 	/* Parent (optional) */
 	struct node *parent;
 
+	/* Generation count. */
+	uint64_t generation;
+
 	/* Permissions. */
 	unsigned int num_perms;
 	struct xs_permissions *perms;
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index b08b2eb..6c65dc5 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -68,7 +68,10 @@ struct transaction
 	uint32_t id;
 
 	/* Generation when transaction started. */
-	unsigned int generation;
+	uint64_t generation;
+
+	/* Transaction internal generation. */
+	uint64_t trans_gen;
 
 	/* TDB to work on, and filename */
 	TDB_CONTEXT *tdb;
@@ -82,7 +85,7 @@ struct transaction
 };
 
 extern int quota_max_transaction;
-static unsigned int generation;
+static uint64_t generation;
 
 /* Return tdb context to use for this connection. */
 TDB_CONTEXT *tdb_transaction_context(struct transaction *trans)
@@ -99,12 +102,14 @@ void add_change_node(struct connection *conn, struct node *node, bool recurse)
 
 	if (!conn || !conn->transaction) {
 		/* They're changing the global database. */
-		generation++;
+		node->generation = generation++;
 		return;
 	}
 
 	trans = conn->transaction;
 
+	node->generation = generation + trans->trans_gen++;
+
 	list_for_each_entry(i, &trans->changes, list) {
 		if (streq(i->node, node->name)) {
 			if (recurse)
@@ -161,7 +166,7 @@ void do_transaction_start(struct connection *conn, struct buffered_data *in)
 	}
 
 	/* Attach transaction to input for autofree until it's complete */
-	trans = talloc(in, struct transaction);
+	trans = talloc_zero(in, struct transaction);
 	INIT_LIST_HEAD(&trans->changes);
 	INIT_LIST_HEAD(&trans->changed_domains);
 	trans->generation = generation;
@@ -235,7 +240,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, in, i->node, i->recurse);
-		generation++;
+		generation += trans->trans_gen;
 	}
 	send_ack(conn, XS_TRANSACTION_END);
 }
-- 
2.10.2


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

Re: [PATCH v4 04/12] xenstore: add per-node generation counter

[Jan Beulich]

>>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
> --- a/tools/xenstore/include/xenstore_lib.h
> +++ b/tools/xenstore/include/xenstore_lib.h
> @@ -44,6 +44,7 @@ struct xs_permissions
>  
>  /* Header of the node record in tdb. */
>  struct xs_tdb_record_hdr {
> +	uint64_t generation;
>  	uint32_t num_perms;
>  	uint32_t datalen;
>  	uint32_t childlen;

After quite a bit of debugging I think I now understand that this is
the reason for a startup SEGV I'm getting from 4.8 xenstored after
that same system has run unstable xenstored. The above
represents a binary change (and, perhaps even worse, one making
the layout no longer match between 32- and 64-bit tool stacks) to
TDB layout, yet the commit didn't bump TDB_VERSION. I'll submit a
patch to bump the version in a minute, but I'll leave it to others to
judge about the layout aspect.

Now that's only the surface part of the problem. Having looked into
check_store_() during debugging, I think it is quite ugly for that
function to infinitely invoke itself recursively if it finds (in my case here,
but I think this is just one example) all zeros as child names (leading to
child_name() producing "/", i.e. the node name its top level invocation
from check_store() passed).

Jan


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

Re: [PATCH v4 04/12] xenstore: add per-node generation counter

[Juergen Gross]

On 09/01/17 15:38, Jan Beulich wrote:
>>>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
>> --- a/tools/xenstore/include/xenstore_lib.h
>> +++ b/tools/xenstore/include/xenstore_lib.h
>> @@ -44,6 +44,7 @@ struct xs_permissions
>>  
>>  /* Header of the node record in tdb. */
>>  struct xs_tdb_record_hdr {
>> +	uint64_t generation;
>>  	uint32_t num_perms;
>>  	uint32_t datalen;
>>  	uint32_t childlen;
> 
> After quite a bit of debugging I think I now understand that this is
> the reason for a startup SEGV I'm getting from 4.8 xenstored after
> that same system has run unstable xenstored. The above

How are you starting xenstored? I can't see how xenstored started
by Xen's scripts will see a file with a tdb dump. Those should be
deleted by the start scripts of Xen.

The only way I can see this would happen is if you kill a running
xenstored and start another one manually. As this procedure will drop
all registered xenstore watches I think we rather should drop support
for starting xenstored with a populated tdb file.


Juergen

> represents a binary change (and, perhaps even worse, one making
> the layout no longer match between 32- and 64-bit tool stacks) to
> TDB layout, yet the commit didn't bump TDB_VERSION. I'll submit a
> patch to bump the version in a minute, but I'll leave it to others to
> judge about the layout aspect.
> 
> Now that's only the surface part of the problem. Having looked into
> check_store_() during debugging, I think it is quite ugly for that
> function to infinitely invoke itself recursively if it finds (in my case here,
> but I think this is just one example) all zeros as child names (leading to
> child_name() producing "/", i.e. the node name its top level invocation
> from check_store() passed).
> 
> Jan
> 


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

Re: [PATCH v4 04/12] xenstore: add per-node generation counter

[Jan Beulich]

>>> On 09.01.17 at 17:02, <JGross@suse.com> wrote:
> On 09/01/17 15:38, Jan Beulich wrote:
>>>>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
>>> --- a/tools/xenstore/include/xenstore_lib.h
>>> +++ b/tools/xenstore/include/xenstore_lib.h
>>> @@ -44,6 +44,7 @@ struct xs_permissions
>>>  
>>>  /* Header of the node record in tdb. */
>>>  struct xs_tdb_record_hdr {
>>> +	uint64_t generation;
>>>  	uint32_t num_perms;
>>>  	uint32_t datalen;
>>>  	uint32_t childlen;
>> 
>> After quite a bit of debugging I think I now understand that this is
>> the reason for a startup SEGV I'm getting from 4.8 xenstored after
>> that same system has run unstable xenstored. The above
> 
> How are you starting xenstored? I can't see how xenstored started
> by Xen's scripts will see a file with a tdb dump. Those should be
> deleted by the start scripts of Xen.

I order to be able to use multiple Xen versions on a single system,
I can't use the start scripts. Instead I launch xenstored from a
script of mine (wrapping all the various xen-specific commands)
upon first use after boot. Of course I could add deletion of that
file to the script, but then again all has worked fine for years, so
I'm a little reluctant to do so (after all I'd expect/hope there's a
reason for this file to exist in the first place).

Jan


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

Re: [PATCH v4 04/12] xenstore: add per-node generation counter

[Juergen Gross]

On 09/01/17 17:10, Jan Beulich wrote:
>>>> On 09.01.17 at 17:02, <JGross@suse.com> wrote:
>> On 09/01/17 15:38, Jan Beulich wrote:
>>>>>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
>>>> --- a/tools/xenstore/include/xenstore_lib.h
>>>> +++ b/tools/xenstore/include/xenstore_lib.h
>>>> @@ -44,6 +44,7 @@ struct xs_permissions
>>>>  
>>>>  /* Header of the node record in tdb. */
>>>>  struct xs_tdb_record_hdr {
>>>> +	uint64_t generation;
>>>>  	uint32_t num_perms;
>>>>  	uint32_t datalen;
>>>>  	uint32_t childlen;
>>>
>>> After quite a bit of debugging I think I now understand that this is
>>> the reason for a startup SEGV I'm getting from 4.8 xenstored after
>>> that same system has run unstable xenstored. The above
>>
>> How are you starting xenstored? I can't see how xenstored started
>> by Xen's scripts will see a file with a tdb dump. Those should be
>> deleted by the start scripts of Xen.
> 
> I order to be able to use multiple Xen versions on a single system,
> I can't use the start scripts. Instead I launch xenstored from a
> script of mine (wrapping all the various xen-specific commands)
> upon first use after boot. Of course I could add deletion of that
> file to the script, but then again all has worked fine for years, so
> I'm a little reluctant to do so (after all I'd expect/hope there's a
> reason for this file to exist in the first place).

I believe this file is meant to aid diagnosis of a crashed xenstored:
you can extract the xenstore entries from it in order to see how far
it proceeded processing the last command.

I suggest to bump the TDB_VERSION in order to avoid something like
this happening again and then to always start with an empty xenstore
(I can write that patch) by not opening an existing tdb file in
xenstored.


Juergen


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

[PATCH v4 05/12] xenstore: add support for reading directory with many children

[Juergen Gross]

As the payload size for one xenstore wire command is limited to 4096
bytes it is impossible to read the children names of a node with a
large number of children (e.g. /local/domain in case of a host with
more than about 2000 domains). This effectively limits the maximum
number of domains a host can support.

In order to support such long directory outputs add a new wire command
XS_DIRECTORY_PART which will return only some entries in each call and
can be called in a loop to get all entries.

Input data are the path of the node and the byte offset into the child
list where returned data should start.

Output is the generation count of the node (which will change each time
the node is being modified) and a list of child names starting with
the specified index. The end of the list is indicated by an empty
child name. It is the responsibility of the caller to check for data
consistency by comparing the generation counts of all returned data
sets to be the same for one node.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
V4: remove XS_NEXT_ENTRY again as requested by Jan Beulich
V3: use genlen, memcpy instead of strcpy as requested by Jan Beulich
    add XS_NEXT_ENTRY to xs_wire.h
    add XS_DIRECTORY_PART to sockmsg_string()
---
 tools/xenstore/xenstored_core.c | 67 +++++++++++++++++++++++++++++++++++++++++
 xen/include/public/io/xs_wire.h |  1 +
 2 files changed, 68 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 95d6d7d..e4e09fa 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -16,6 +16,7 @@
     along with this program; If not, see <http://www.gnu.org/licenses/>.
 */
 
+#include <inttypes.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <poll.h>
@@ -147,6 +148,7 @@ static char *sockmsg_string(enum xsd_sockmsg_type type)
 	case XS_RESUME: return "RESUME";
 	case XS_SET_TARGET: return "SET_TARGET";
 	case XS_RESET_WATCHES: return "RESET_WATCHES";
+	case XS_DIRECTORY_PART: return "DIRECTORY_PART";
 	default:
 		return "**UNKNOWN**";
 	}
@@ -812,6 +814,67 @@ static void send_directory(struct connection *conn, struct buffered_data *in)
 	send_reply(conn, XS_DIRECTORY, node->children, node->childlen);
 }
 
+static void send_directory_part(struct connection *conn,
+				struct buffered_data *in)
+{
+	unsigned int off, len, maxlen, genlen;
+	char *name, *child, *data;
+	struct node *node;
+	char gen[24];
+
+	if (xs_count_strings(in->buffer, in->used) != 2) {
+		send_error(conn, EINVAL);
+		return;
+	}
+
+	/* First arg is node name. */
+	name = canonicalize(conn, in->buffer);
+
+	/* Second arg is childlist offset. */
+	off = atoi(in->buffer + strlen(in->buffer) + 1);
+
+	node = get_node(conn, in, name, XS_PERM_READ);
+	if (!node) {
+		send_error(conn, errno);
+		return;
+	}
+
+	genlen = snprintf(gen, sizeof(gen), "%"PRIu64, node->generation) + 1;
+
+	/* Offset behind list: just return a list with an empty string. */
+	if (off >= node->childlen) {
+		gen[genlen] = 0;
+		send_reply(conn, XS_DIRECTORY_PART, gen, genlen + 1);
+		return;
+	}
+
+	len = 0;
+	maxlen = XENSTORE_PAYLOAD_MAX - genlen - 1;
+	child = node->children + off;
+
+	while (len + strlen(child) < maxlen) {
+		len += strlen(child) + 1;
+		child += strlen(child) + 1;
+		if (off + len == node->childlen)
+			break;
+	}
+
+	data = talloc_array(in, char, genlen + len + 1);
+	if (!data) {
+		send_error(conn, ENOMEM);
+		return;
+	}
+
+	memcpy(data, gen, genlen);
+	memcpy(data + genlen, node->children + off, len);
+	if (off + len == node->childlen) {
+		data[genlen + len] = 0;
+		len++;
+	}
+
+	send_reply(conn, XS_DIRECTORY_PART, data, genlen + len);
+}
+
 static void do_read(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
@@ -1334,6 +1397,10 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 		do_reset_watches(conn, in);
 		break;
 
+	case XS_DIRECTORY_PART:
+		send_directory_part(conn, in);
+		break;
+
 	default:
 		eprintf("Client unknown operation %i", in->hdr.msg.type);
 		send_error(conn, ENOSYS);
diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index 0a0cdbc..545f916 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -50,6 +50,7 @@ enum xsd_sockmsg_type
     XS_SET_TARGET,
     XS_RESTRICT,
     XS_RESET_WATCHES,
+    XS_DIRECTORY_PART,
 
     XS_INVALID = 0xffff /* Guaranteed to remain an invalid type */
 };
-- 
2.10.2


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

[PATCH v4 05/12] xenstore: add support for reading directory with many children

[Ian Jackson]

Juergen Gross writes ("[PATCH v4 05/12] xenstore: add support for reading directory with many children"):
> Output is the generation count of the node (which will change each time
> the node is being modified) and a list of child names starting with
> the specified index. The end of the list is indicated by an empty
> child name. It is the responsibility of the caller to check for data
> consistency by comparing the generation counts of all returned data
> sets to be the same for one node.

Why not require the caller to run this command in a transaction ?

Ian.

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

Re: [PATCH v4 05/12] xenstore: add support for reading directory with many children

[Juergen Gross]

On 09/01/17 17:40, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH v4 05/12] xenstore: add support for reading directory with many children"):
>> Output is the generation count of the node (which will change each time
>> the node is being modified) and a list of child names starting with
>> the specified index. The end of the list is indicated by an empty
>> child name. It is the responsibility of the caller to check for data
>> consistency by comparing the generation counts of all returned data
>> sets to be the same for one node.
> 
> Why not require the caller to run this command in a transaction ?

You are a little bit late. This patch has been applied.

And yes, that was my idea first. Jan thought the current solution to be
better.


Juergen


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

[PATCH v4 06/12] xenstore: support XS_DIRECTORY_PART in libxenstore

[Juergen Gross]

This will enable all users of libxenstore to handle xenstore nodes
with a huge amount of children.

In order to not depend completely on the XS_DIRECTORY_PART
functionality use it only in case of E2BIG returned by XS_DIRECTORY.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xs.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index d1e01ba..40e3275 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -558,15 +558,10 @@ static bool xs_bool(char *reply)
 	return true;
 }
 
-char **xs_directory(struct xs_handle *h, xs_transaction_t t,
-		    const char *path, unsigned int *num)
+static char **xs_directory_common(char *strings, unsigned int len,
+				  unsigned int *num)
 {
-	char *strings, *p, **ret;
-	unsigned int len;
-
-	strings = xs_single(h, t, XS_DIRECTORY, path, &len);
-	if (!strings)
-		return NULL;
+	char *p, **ret;
 
 	/* Count the strings. */
 	*num = xs_count_strings(strings, len);
@@ -586,6 +581,75 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t,
 	return ret;
 }
 
+static char **xs_directory_part(struct xs_handle *h, xs_transaction_t t,
+				const char *path, unsigned int *num)
+{
+	unsigned int off, result_len;
+	char gen[24], offstr[8];
+	struct iovec iovec[2];
+	char *result = NULL, *strings = NULL;
+
+	gen[0] = 0;
+	iovec[0].iov_base = (void *)path;
+	iovec[0].iov_len = strlen(path) + 1;
+
+	for (off = 0;;) {
+		snprintf(offstr, sizeof(offstr), "%u", off);
+		iovec[1].iov_base = (void *)offstr;
+		iovec[1].iov_len = strlen(offstr) + 1;
+		result = xs_talkv(h, t, XS_DIRECTORY_PART, iovec, 2,
+				  &result_len);
+
+		/* If XS_DIRECTORY_PART isn't supported return E2BIG. */
+		if (!result) {
+			if (errno == ENOSYS)
+				errno = E2BIG;
+			return NULL;
+		}
+
+		if (off) {
+			if (strcmp(gen, result)) {
+				free(result);
+				free(strings);
+				strings = NULL;
+				off = 0;
+				continue;
+			}
+		} else
+			strncpy(gen, result, sizeof(gen));
+
+		result_len -= strlen(result) + 1;
+		strings = realloc(strings, off + result_len);
+		memcpy(strings + off, result + strlen(result) + 1, result_len);
+		free(result);
+		off += result_len;
+
+		if (off <= 1 || strings[off - 2] == 0)
+			break;
+	}
+
+	if (off > 1)
+		off--;
+
+	return xs_directory_common(strings, off, num);
+}
+
+char **xs_directory(struct xs_handle *h, xs_transaction_t t,
+		    const char *path, unsigned int *num)
+{
+	char *strings;
+	unsigned int len;
+
+	strings = xs_single(h, t, XS_DIRECTORY, path, &len);
+	if (!strings) {
+		if (errno != E2BIG)
+			return NULL;
+		return xs_directory_part(h, t, path, num);
+	}
+
+	return xs_directory_common(strings, len, num);
+}
+
 /* Get the value of a single file, nul terminated.
  * Returns a malloced value: call free() on it after use.
  * len indicates length in bytes, not including the nul.
-- 
2.10.2


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

[PATCH v4 07/12] xenstore: use array for xenstore wire command handling

[Juergen Gross]

Instead of switch() statements for selecting wire command actions use
an array for this purpose.

While doing this add the XS_RESTRICT type for diagnostic prints and
correct the printed string for XS_IS_DOMAIN_INTRODUCED.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
V4: add XS_TYPE_COUNT (moved from patch 5) as requested by Jan Beulich
---
 tools/xenstore/xenstored_core.c | 158 +++++++++++-----------------------------
 xen/include/public/io/xs_wire.h |   2 +
 2 files changed, 46 insertions(+), 114 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index e4e09fa..23e3771 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -86,6 +86,7 @@ static bool trigger_talloc_report = false;
 
 static void corrupt(struct connection *conn, const char *fmt, ...);
 static void check_store(void);
+static char *sockmsg_string(enum xsd_sockmsg_type type);
 
 #define log(...)							\
 	do {								\
@@ -124,36 +125,6 @@ bool replace_tdb(const char *newname, TDB_CONTEXT *newtdb)
 	return true;
 }
 
-static char *sockmsg_string(enum xsd_sockmsg_type type)
-{
-	switch (type) {
-	case XS_DEBUG: return "DEBUG";
-	case XS_DIRECTORY: return "DIRECTORY";
-	case XS_READ: return "READ";
-	case XS_GET_PERMS: return "GET_PERMS";
-	case XS_WATCH: return "WATCH";
-	case XS_UNWATCH: return "UNWATCH";
-	case XS_TRANSACTION_START: return "TRANSACTION_START";
-	case XS_TRANSACTION_END: return "TRANSACTION_END";
-	case XS_INTRODUCE: return "INTRODUCE";
-	case XS_RELEASE: return "RELEASE";
-	case XS_GET_DOMAIN_PATH: return "GET_DOMAIN_PATH";
-	case XS_WRITE: return "WRITE";
-	case XS_MKDIR: return "MKDIR";
-	case XS_RM: return "RM";
-	case XS_SET_PERMS: return "SET_PERMS";
-	case XS_WATCH_EVENT: return "WATCH_EVENT";
-	case XS_ERROR: return "ERROR";
-	case XS_IS_DOMAIN_INTRODUCED: return "XS_IS_DOMAIN_INTRODUCED";
-	case XS_RESUME: return "RESUME";
-	case XS_SET_TARGET: return "SET_TARGET";
-	case XS_RESET_WATCHES: return "RESET_WATCHES";
-	case XS_DIRECTORY_PART: return "DIRECTORY_PART";
-	default:
-		return "**UNKNOWN**";
-	}
-}
-
 void trace(const char *fmt, ...)
 {
 	va_list arglist;
@@ -1304,12 +1275,51 @@ static void do_debug(struct connection *conn, struct buffered_data *in)
 	send_ack(conn, XS_DEBUG);
 }
 
+static struct {
+	char *str;
+	void (*func)(struct connection *conn, struct buffered_data *in);
+} wire_funcs[XS_TYPE_COUNT] = {
+	[XS_DEBUG]             = { "DEBUG",             do_debug },
+	[XS_DIRECTORY]         = { "DIRECTORY",         send_directory },
+	[XS_READ]              = { "READ",              do_read },
+	[XS_GET_PERMS]         = { "GET_PERMS",         do_get_perms },
+	[XS_WATCH]             = { "WATCH",             do_watch },
+	[XS_UNWATCH]           = { "UNWATCH",           do_unwatch },
+	[XS_TRANSACTION_START] = { "TRANSACTION_START", do_transaction_start },
+	[XS_TRANSACTION_END]   = { "TRANSACTION_END",   do_transaction_end },
+	[XS_INTRODUCE]         = { "INTRODUCE",         do_introduce },
+	[XS_RELEASE]           = { "RELEASE",           do_release },
+	[XS_GET_DOMAIN_PATH]   = { "GET_DOMAIN_PATH",   do_get_domain_path },
+	[XS_WRITE]             = { "WRITE",             do_write },
+	[XS_MKDIR]             = { "MKDIR",             do_mkdir },
+	[XS_RM]                = { "RM",                do_rm },
+	[XS_SET_PERMS]         = { "SET_PERMS",         do_set_perms },
+	[XS_WATCH_EVENT]       = { "WATCH_EVENT",       NULL },
+	[XS_ERROR]             = { "ERROR",             NULL },
+	[XS_IS_DOMAIN_INTRODUCED] =
+			{ "IS_DOMAIN_INTRODUCED", do_is_domain_introduced },
+	[XS_RESUME]            = { "RESUME",            do_resume },
+	[XS_SET_TARGET]        = { "SET_TARGET",        do_set_target },
+	[XS_RESTRICT]          = { "RESTRICT",          NULL },
+	[XS_RESET_WATCHES]     = { "RESET_WATCHES",     do_reset_watches },
+	[XS_DIRECTORY_PART]    = { "DIRECTORY_PART",    send_directory_part },
+};
+
+static char *sockmsg_string(enum xsd_sockmsg_type type)
+{
+	if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].str)
+		return wire_funcs[type].str;
+
+	return "**UNKNOWN**";
+}
+
 /* Process "in" for conn: "in" will vanish after this conversation, so
  * we can talloc off it for temporary variables.  May free "conn".
  */
 static void process_message(struct connection *conn, struct buffered_data *in)
 {
 	struct transaction *trans;
+	enum xsd_sockmsg_type type = in->hdr.msg.type;
 
 	trans = transaction_lookup(conn, in->hdr.msg.tx_id);
 	if (IS_ERR(trans)) {
@@ -1320,91 +1330,11 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 	assert(conn->transaction == NULL);
 	conn->transaction = trans;
 
-	switch (in->hdr.msg.type) {
-	case XS_DIRECTORY:
-		send_directory(conn, in);
-		break;
-
-	case XS_READ:
-		do_read(conn, in);
-		break;
-
-	case XS_WRITE:
-		do_write(conn, in);
-		break;
-
-	case XS_MKDIR:
-		do_mkdir(conn, in);
-		break;
-
-	case XS_RM:
-		do_rm(conn, in);
-		break;
-
-	case XS_GET_PERMS:
-		do_get_perms(conn, in);
-		break;
-
-	case XS_SET_PERMS:
-		do_set_perms(conn, in);
-		break;
-
-	case XS_DEBUG:
-		do_debug(conn, in);
-		break;
-
-	case XS_WATCH:
-		do_watch(conn, in);
-		break;
-
-	case XS_UNWATCH:
-		do_unwatch(conn, in);
-		break;
-
-	case XS_TRANSACTION_START:
-		do_transaction_start(conn, in);
-		break;
-
-	case XS_TRANSACTION_END:
-		do_transaction_end(conn, in);
-		break;
-
-	case XS_INTRODUCE:
-		do_introduce(conn, in);
-		break;
-
-	case XS_IS_DOMAIN_INTRODUCED:
-		do_is_domain_introduced(conn, in);
-		break;
-
-	case XS_RELEASE:
-		do_release(conn, in);
-		break;
-
-	case XS_GET_DOMAIN_PATH:
-		do_get_domain_path(conn, in);
-		break;
-
-	case XS_RESUME:
-		do_resume(conn, in);
-		break;
-
-	case XS_SET_TARGET:
-		do_set_target(conn, in);
-		break;
-
-	case XS_RESET_WATCHES:
-		do_reset_watches(conn, in);
-		break;
-
-	case XS_DIRECTORY_PART:
-		send_directory_part(conn, in);
-		break;
-
-	default:
-		eprintf("Client unknown operation %i", in->hdr.msg.type);
+	if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].func)
+		wire_funcs[type].func(conn, in);
+	else {
+		eprintf("Client unknown operation %i", type);
 		send_error(conn, ENOSYS);
-		break;
 	}
 
 	conn->transaction = NULL;
diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index 545f916..54c1d71 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -52,6 +52,8 @@ enum xsd_sockmsg_type
     XS_RESET_WATCHES,
     XS_DIRECTORY_PART,
 
+    XS_TYPE_COUNT,      /* Number of valid types. */
+
     XS_INVALID = 0xffff /* Guaranteed to remain an invalid type */
 };
 
-- 
2.10.2


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

[PATCH v4 07/12] xenstore: use array for xenstore wire command handling

[Jan Beulich]

>>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
> @@ -1304,12 +1275,51 @@ static void do_debug(struct connection *conn, struct buffered_data *in)
>  	send_ack(conn, XS_DEBUG);
>  }
>  
> +static struct {
> +	char *str;
> +	void (*func)(struct connection *conn, struct buffered_data *in);
> +} wire_funcs[XS_TYPE_COUNT] = {

If this was hypervisor code, I would demand both the array as a
whole and the str member to become constified. Not sure what the
tool stack side non-library policy is.

> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -52,6 +52,8 @@ enum xsd_sockmsg_type
>      XS_RESET_WATCHES,
>      XS_DIRECTORY_PART,
>  
> +    XS_TYPE_COUNT,      /* Number of valid types. */
> +
>      XS_INVALID = 0xffff /* Guaranteed to remain an invalid type */
>  };

This as well as the XS_DIRECTORY_PART addition in patch 5 can
have my ack, if needed (albeit I guess the tool stack maintainers'
will do).

Jan


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

Re: [PATCH v4 07/12] xenstore: use array for xenstore wire command handling

[Wei Liu]

On Mon, Dec 05, 2016 at 01:43:40AM -0700, Jan Beulich wrote:
> >>> On 05.12.16 at 08:48, <JGross@suse.com> wrote:
> > @@ -1304,12 +1275,51 @@ static void do_debug(struct connection *conn, struct buffered_data *in)
> >  	send_ack(conn, XS_DEBUG);
> >  }
> >  
> > +static struct {
> > +	char *str;
> > +	void (*func)(struct connection *conn, struct buffered_data *in);
> > +} wire_funcs[XS_TYPE_COUNT] = {
> 
> If this was hypervisor code, I would demand both the array as a
> whole and the str member to become constified. Not sure what the
> tool stack side non-library policy is.
> 

I think that's a good idea. No need to resend for this. I can change it
while committing.

Wei.

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

[PATCH v4 08/12] xenstore: let command functions return error or success

[Juergen Gross]

Add a return value to all wire command functions of xenstored. If such
a function returns an error send the error message in
process_message().

Only code refactoring, no change in behavior expected.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c        | 213 +++++++++++++++------------------
 tools/xenstore/xenstored_domain.c      | 170 +++++++++++---------------
 tools/xenstore/xenstored_domain.h      |  14 +--
 tools/xenstore/xenstored_transaction.c |  50 ++++----
 tools/xenstore/xenstored_transaction.h |   4 +-
 tools/xenstore/xenstored_watch.c       |  46 +++----
 tools/xenstore/xenstored_watch.h       |   4 +-
 7 files changed, 212 insertions(+), 289 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 23e3771..938b652 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -770,33 +770,31 @@ bool check_event_node(const char *node)
 	return true;
 }
 
-static void send_directory(struct connection *conn, struct buffered_data *in)
+static int 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, in, name, XS_PERM_READ);
-	if (!node) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!node)
+		return errno;
 
 	send_reply(conn, XS_DIRECTORY, node->children, node->childlen);
+
+	return 0;
 }
 
-static void send_directory_part(struct connection *conn,
-				struct buffered_data *in)
+static int send_directory_part(struct connection *conn,
+			       struct buffered_data *in)
 {
 	unsigned int off, len, maxlen, genlen;
 	char *name, *child, *data;
 	struct node *node;
 	char gen[24];
 
-	if (xs_count_strings(in->buffer, in->used) != 2) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (xs_count_strings(in->buffer, in->used) != 2)
+		return EINVAL;
 
 	/* First arg is node name. */
 	name = canonicalize(conn, in->buffer);
@@ -805,10 +803,8 @@ static void send_directory_part(struct connection *conn,
 	off = atoi(in->buffer + strlen(in->buffer) + 1);
 
 	node = get_node(conn, in, name, XS_PERM_READ);
-	if (!node) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!node)
+		return errno;
 
 	genlen = snprintf(gen, sizeof(gen), "%"PRIu64, node->generation) + 1;
 
@@ -816,7 +812,7 @@ static void send_directory_part(struct connection *conn,
 	if (off >= node->childlen) {
 		gen[genlen] = 0;
 		send_reply(conn, XS_DIRECTORY_PART, gen, genlen + 1);
-		return;
+		return 0;
 	}
 
 	len = 0;
@@ -831,10 +827,8 @@ static void send_directory_part(struct connection *conn,
 	}
 
 	data = talloc_array(in, char, genlen + len + 1);
-	if (!data) {
-		send_error(conn, ENOMEM);
-		return;
-	}
+	if (!data)
+		return ENOMEM;
 
 	memcpy(data, gen, genlen);
 	memcpy(data + genlen, node->children + off, len);
@@ -844,21 +838,23 @@ static void send_directory_part(struct connection *conn,
 	}
 
 	send_reply(conn, XS_DIRECTORY_PART, data, genlen + len);
+
+	return 0;
 }
 
-static void do_read(struct connection *conn, struct buffered_data *in)
+static int 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, in, name, XS_PERM_READ);
-	if (!node) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!node)
+		return errno;
 
 	send_reply(conn, XS_READ, node->data, node->datalen);
+
+	return 0;
 }
 
 static void delete_node_single(struct connection *conn, struct node *node,
@@ -977,7 +973,7 @@ static struct node *create_node(struct connection *conn,
 }
 
 /* path, data... */
-static void do_write(struct connection *conn, struct buffered_data *in)
+static int do_write(struct connection *conn, struct buffered_data *in)
 {
 	unsigned int offset, datalen;
 	struct node *node;
@@ -985,10 +981,8 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	char *name;
 
 	/* Extra "strings" can be created by binary data. */
-	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
+		return EINVAL;
 
 	offset = strlen(vec[0]) + 1;
 	datalen = in->used - offset;
@@ -997,38 +991,31 @@ static void do_write(struct connection *conn, struct buffered_data *in)
 	node = get_node(conn, in, name, XS_PERM_WRITE);
 	if (!node) {
 		/* No permissions, invalid input? */
-		if (errno != ENOENT) {
-			send_error(conn, errno);
-			return;
-		}
+		if (errno != ENOENT)
+			return errno;
 		node = create_node(conn, name, in->buffer + offset, datalen);
-		if (!node) {
-			send_error(conn, errno);
-			return;
-		}
+		if (!node)
+			return errno;
 	} else {
 		node->data = in->buffer + offset;
 		node->datalen = datalen;
-		if (!write_node(conn, node)){
-			send_error(conn, errno);
-			return;
-		}
+		if (!write_node(conn, node))
+			return errno;
 	}
 
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_WRITE);
+
+	return 0;
 }
 
-static void do_mkdir(struct connection *conn, struct buffered_data *in)
+static int do_mkdir(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
 	const char *name = onearg(in);
 
-	if (!name) {
-		errno = EINVAL;
-		send_error(conn, errno);
-		return;
-	}
+	if (!name)
+		return EINVAL;
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, in, name, XS_PERM_WRITE);
@@ -1036,18 +1023,16 @@ static void do_mkdir(struct connection *conn, struct buffered_data *in)
 	/* If it already exists, fine. */
 	if (!node) {
 		/* No permissions? */
-		if (errno != ENOENT) {
-			send_error(conn, errno);
-			return;
-		}
+		if (errno != ENOENT)
+			return errno;
 		node = create_node(conn, name, NULL, 0);
-		if (!node) {
-			send_error(conn, errno);
-			return;
-		}
+		if (!node)
+			return errno;
 		fire_watches(conn, in, name, false);
 	}
 	send_ack(conn, XS_MKDIR);
+
+	return 0;
 }
 
 static void delete_node(struct connection *conn, struct node *node,
@@ -1117,18 +1102,14 @@ static int _rm(struct connection *conn, struct node *node, const char *name)
 	   happen is the child will continue to take up space, but will
 	   otherwise be unreachable. */
 	struct node *parent = read_node(conn, name, get_parent(name, name));
-	if (!parent) {
-		send_error(conn, EINVAL);
-		return 0;
-	}
+	if (!parent)
+		return EINVAL;
 
-	if (!delete_child(conn, parent, basename(name))) {
-		send_error(conn, EINVAL);
-		return 0;
-	}
+	if (!delete_child(conn, parent, basename(name)))
+		return EINVAL;
 
 	delete_node(conn, node, true);
-	return 1;
+	return 0;
 }
 
 
@@ -1143,9 +1124,10 @@ static void internal_rm(const char *name)
 }
 
 
-static void do_rm(struct connection *conn, struct buffered_data *in)
+static int do_rm(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
+	int ret;
 	const char *name = onearg(in);
 
 	name = canonicalize(conn, name);
@@ -1156,28 +1138,29 @@ static void do_rm(struct connection *conn, struct buffered_data *in)
 			node = read_node(conn, in, get_parent(in, name));
 			if (node) {
 				send_ack(conn, XS_RM);
-				return;
+				return 0;
 			}
 			/* Restore errno, just in case. */
 			errno = ENOENT;
 		}
-		send_error(conn, errno);
-		return;
+		return errno;
 	}
 
-	if (streq(name, "/")) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (streq(name, "/"))
+		return EINVAL;
 
-	if (_rm(conn, node, name)) {
-		fire_watches(conn, in, name, true);
-		send_ack(conn, XS_RM);
-	}
+	ret = _rm(conn, node, name);
+	if (ret)
+		return ret;
+
+	fire_watches(conn, in, name, true);
+	send_ack(conn, XS_RM);
+
+	return 0;
 }
 
 
-static void do_get_perms(struct connection *conn, struct buffered_data *in)
+static int do_get_perms(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
 	const char *name = onearg(in);
@@ -1186,19 +1169,19 @@ static void do_get_perms(struct connection *conn, struct buffered_data *in)
 
 	name = canonicalize(conn, name);
 	node = get_node(conn, in, name, XS_PERM_READ);
-	if (!node) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!node)
+		return errno;
 
 	strings = perms_to_strings(node, node->perms, node->num_perms, &len);
 	if (!strings)
-		send_error(conn, errno);
-	else
-		send_reply(conn, XS_GET_PERMS, strings, len);
+		return errno;
+
+	send_reply(conn, XS_GET_PERMS, strings, len);
+
+	return 0;
 }
 
-static void do_set_perms(struct connection *conn, struct buffered_data *in)
+static int do_set_perms(struct connection *conn, struct buffered_data *in)
 {
 	unsigned int num;
 	struct xs_permissions *perms;
@@ -1206,10 +1189,8 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 	struct node *node;
 
 	num = xs_count_strings(in->buffer, in->used);
-	if (num < 2) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (num < 2)
+		return EINVAL;
 
 	/* First arg is node name. */
 	name = canonicalize(conn, in->buffer);
@@ -1218,54 +1199,43 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
 
 	/* We must own node to do this (tools can do this too). */
 	node = get_node(conn, in, name, XS_PERM_WRITE|XS_PERM_OWNER);
-	if (!node) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!node)
+		return errno;
 
 	perms = talloc_array(node, struct xs_permissions, num);
-	if (!xs_strings_to_perms(perms, num, permstr)) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!xs_strings_to_perms(perms, num, permstr))
+		return errno;
 
 	/* Unprivileged domains may not change the owner. */
-	if (domain_is_unprivileged(conn) &&
-	    perms[0].id != node->perms[0].id) {
-		send_error(conn, EPERM);
-		return;
-	}
+	if (domain_is_unprivileged(conn) && perms[0].id != node->perms[0].id)
+		return EPERM;
 
 	domain_entry_dec(conn, node);
 	node->perms = perms;
 	node->num_perms = num;
 	domain_entry_inc(conn, node);
 
-	if (!write_node(conn, node)) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!write_node(conn, node))
+		return errno;
 
 	fire_watches(conn, in, name, false);
 	send_ack(conn, XS_SET_PERMS);
+
+	return 0;
 }
 
-static void do_debug(struct connection *conn, struct buffered_data *in)
+static int do_debug(struct connection *conn, struct buffered_data *in)
 {
 	int num;
 
-	if (conn->id != 0) {
-		send_error(conn, EACCES);
-		return;
-	}
+	if (conn->id != 0)
+		return EACCES;
 
 	num = xs_count_strings(in->buffer, in->used);
 
 	if (streq(in->buffer, "print")) {
-		if (num < 2) {
-			send_error(conn, EINVAL);
-			return;
-		}
+		if (num < 2)
+			return EINVAL;
 		xprintf("debug: %s", in->buffer + get_string(in, 0));
 	}
 
@@ -1273,11 +1243,13 @@ static void do_debug(struct connection *conn, struct buffered_data *in)
 		check_store();
 
 	send_ack(conn, XS_DEBUG);
+
+	return 0;
 }
 
 static struct {
 	char *str;
-	void (*func)(struct connection *conn, struct buffered_data *in);
+	int (*func)(struct connection *conn, struct buffered_data *in);
 } wire_funcs[XS_TYPE_COUNT] = {
 	[XS_DEBUG]             = { "DEBUG",             do_debug },
 	[XS_DIRECTORY]         = { "DIRECTORY",         send_directory },
@@ -1320,6 +1292,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 {
 	struct transaction *trans;
 	enum xsd_sockmsg_type type = in->hdr.msg.type;
+	int ret;
 
 	trans = transaction_lookup(conn, in->hdr.msg.tx_id);
 	if (IS_ERR(trans)) {
@@ -1331,11 +1304,13 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 	conn->transaction = trans;
 
 	if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].func)
-		wire_funcs[type].func(conn, in);
+		ret = wire_funcs[type].func(conn, in);
 	else {
 		eprintf("Client unknown operation %i", type);
-		send_error(conn, ENOSYS);
+		ret = ENOSYS;
 	}
+	if (ret)
+		send_error(conn, ret);
 
 	conn->transaction = NULL;
 }
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 5de93d4..0b2af13 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -338,7 +338,7 @@ static void domain_conn_reset(struct domain *domain)
 }
 
 /* domid, mfn, evtchn, path */
-void do_introduce(struct connection *conn, struct buffered_data *in)
+int do_introduce(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
 	char *vec[3];
@@ -348,40 +348,32 @@ void do_introduce(struct connection *conn, struct buffered_data *in)
 	int rc;
 	struct xenstore_domain_interface *interface;
 
-	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
+		return EINVAL;
 
-	if (domain_is_unprivileged(conn) || !conn->can_write) {
-		send_error(conn, EACCES);
-		return;
-	}
+	if (domain_is_unprivileged(conn) || !conn->can_write)
+		return EACCES;
 
 	domid = atoi(vec[0]);
 	mfn = atol(vec[1]);
 	port = atoi(vec[2]);
 
 	/* Sanity check args. */
-	if (port <= 0) { 
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (port <= 0)
+		return EINVAL;
 
 	domain = find_domain_by_domid(domid);
 
 	if (domain == NULL) {
 		interface = map_interface(domid, mfn);
-		if (!interface) {
-			send_error(conn, errno);
-			return;
-		}
+		if (!interface)
+			return errno;
 		/* Hang domain off "in" until we're finished. */
 		domain = new_domain(in, domid, port);
 		if (!domain) {
+			rc = errno;
 			unmap_interface(interface);
-			send_error(conn, errno);
-			return;
+			return rc;
 		}
 		domain->interface = interface;
 		domain->mfn = mfn;
@@ -397,165 +389,137 @@ void do_introduce(struct connection *conn, struct buffered_data *in)
 		rc = xenevtchn_bind_interdomain(xce_handle, domid, port);
 		domain->port = (rc == -1) ? 0 : rc;
 		domain->remote_port = port;
-	} else {
-		send_error(conn, EINVAL);
-		return;
-	}
+	} else
+		return EINVAL;
 
 	domain_conn_reset(domain);
 
 	send_ack(conn, XS_INTRODUCE);
+
+	return 0;
 }
 
-void do_set_target(struct connection *conn, struct buffered_data *in)
+int do_set_target(struct connection *conn, struct buffered_data *in)
 {
 	char *vec[2];
 	unsigned int domid, tdomid;
         struct domain *domain, *tdomain;
-	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
+		return EINVAL;
 
-	if (domain_is_unprivileged(conn) || !conn->can_write) {
-		send_error(conn, EACCES);
-		return;
-	}
+	if (domain_is_unprivileged(conn) || !conn->can_write)
+		return EACCES;
 
 	domid = atoi(vec[0]);
 	tdomid = atoi(vec[1]);
 
         domain = find_domain_by_domid(domid);
-	if (!domain) {
-		send_error(conn, ENOENT);
-		return;
-	}
-        if (!domain->conn) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domain)
+		return ENOENT;
+        if (!domain->conn)
+		return EINVAL;
 
         tdomain = find_domain_by_domid(tdomid);
-	if (!tdomain) {
-		send_error(conn, ENOENT);
-		return;
-	}
+	if (!tdomain)
+		return ENOENT;
 
-        if (!tdomain->conn) {
-		send_error(conn, EINVAL);
-		return;
-	}
+        if (!tdomain->conn)
+		return EINVAL;
 
         talloc_reference(domain->conn, tdomain->conn);
         domain->conn->target = tdomain->conn;
 
 	send_ack(conn, XS_SET_TARGET);
+
+	return 0;
 }
 
 /* domid */
-void do_release(struct connection *conn, struct buffered_data *in)
+int do_release(struct connection *conn, struct buffered_data *in)
 {
 	const char *domid_str = onearg(in);
 	struct domain *domain;
 	unsigned int domid;
 
-	if (!domid_str) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domid_str)
+		return EINVAL;
 
 	domid = atoi(domid_str);
-	if (!domid) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domid)
+		return EINVAL;
 
-	if (domain_is_unprivileged(conn)) {
-		send_error(conn, EACCES);
-		return;
-	}
+	if (domain_is_unprivileged(conn))
+		return EACCES;
 
 	domain = find_domain_by_domid(domid);
-	if (!domain) {
-		send_error(conn, ENOENT);
-		return;
-	}
+	if (!domain)
+		return ENOENT;
 
-	if (!domain->conn) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domain->conn)
+		return EINVAL;
 
 	talloc_free(domain->conn);
 
 	send_ack(conn, XS_RELEASE);
+
+	return 0;
 }
 
-void do_resume(struct connection *conn, struct buffered_data *in)
+int 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);
-		return;
-	}
+	if (!domid_str)
+		return EINVAL;
 
 	domid = atoi(domid_str);
-	if (!domid) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domid)
+		return EINVAL;
 
-	if (domain_is_unprivileged(conn)) {
-		send_error(conn, EACCES);
-		return;
-	}
+	if (domain_is_unprivileged(conn))
+		return EACCES;
 
 	domain = find_domain_by_domid(domid);
-	if (!domain) {
-		send_error(conn, ENOENT);
-		return;
-	}
+	if (!domain)
+		return ENOENT;
 
-	if (!domain->conn) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!domain->conn)
+		return EINVAL;
 
 	domain->shutdown = 0;
 	
 	send_ack(conn, XS_RESUME);
+
+	return 0;
 }
 
-void do_get_domain_path(struct connection *conn, struct buffered_data *in)
+int 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);
-		return;
-	}
+	if (!domid_str)
+		return EINVAL;
 
 	path = talloc_domain_path(conn, atoi(domid_str));
 
 	send_reply(conn, XS_GET_DOMAIN_PATH, path, strlen(path) + 1);
 
 	talloc_free(path);
+
+	return 0;
 }
 
-void do_is_domain_introduced(struct connection *conn, struct buffered_data *in)
+int 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);
-		return;
-	}
+	if (!domid_str)
+		return EINVAL;
 
 	domid = atoi(domid_str);
 	if (domid == DOMID_SELF)
@@ -564,15 +528,19 @@ void do_is_domain_introduced(struct connection *conn, struct buffered_data *in)
 		result = (find_domain_by_domid(domid) != NULL);
 
 	send_reply(conn, XS_IS_DOMAIN_INTRODUCED, result ? "T" : "F", 2);
+
+	return 0;
 }
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn, struct buffered_data *in)
+int do_reset_watches(struct connection *conn, struct buffered_data *in)
 {
 	conn_delete_all_watches(conn);
 	conn_delete_all_transactions(conn);
 
 	send_ack(conn, XS_RESET_WATCHES);
+
+	return 0;
 }
 
 static int close_xc_handle(void *_handle)
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 2554423..40e15d1 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -22,25 +22,25 @@
 void handle_event(void);
 
 /* domid, mfn, eventchn, path */
-void do_introduce(struct connection *conn, struct buffered_data *in);
+int do_introduce(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_is_domain_introduced(struct connection *conn, struct buffered_data *in);
+int do_is_domain_introduced(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_release(struct connection *conn, struct buffered_data *in);
+int do_release(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_resume(struct connection *conn, struct buffered_data *in);
+int do_resume(struct connection *conn, struct buffered_data *in);
 
 /* domid, target */
-void do_set_target(struct connection *conn, struct buffered_data *in);
+int do_set_target(struct connection *conn, struct buffered_data *in);
 
 /* domid */
-void do_get_domain_path(struct connection *conn, struct buffered_data *in);
+int do_get_domain_path(struct connection *conn, struct buffered_data *in);
 
 /* Allow guest to reset all watches */
-void do_reset_watches(struct connection *conn, struct buffered_data *in);
+int 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 6c65dc5..423fd3e 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -149,21 +149,17 @@ struct transaction *transaction_lookup(struct connection *conn, uint32_t id)
 	return ERR_PTR(-ENOENT);
 }
 
-void do_transaction_start(struct connection *conn, struct buffered_data *in)
+int do_transaction_start(struct connection *conn, struct buffered_data *in)
 {
 	struct transaction *trans, *exists;
 	char id_str[20];
 
 	/* We don't support nested transactions. */
-	if (conn->transaction) {
-		send_error(conn, EBUSY);
-		return;
-	}
+	if (conn->transaction)
+		return EBUSY;
 
-	if (conn->id && conn->transaction_started > quota_max_transaction) {
-		send_error(conn, ENOSPC);
-		return;
-	}
+	if (conn->id && conn->transaction_started > quota_max_transaction)
+		return ENOSPC;
 
 	/* Attach transaction to input for autofree until it's complete */
 	trans = talloc_zero(in, struct transaction);
@@ -173,10 +169,8 @@ void do_transaction_start(struct connection *conn, struct buffered_data *in)
 	trans->tdb_name = talloc_asprintf(trans, "%s.%p",
 					  xs_daemon_tdb(), trans);
 	trans->tdb = tdb_copy(tdb_context(conn), trans->tdb_name);
-	if (!trans->tdb) {
-		send_error(conn, errno);
-		return;
-	}
+	if (!trans->tdb)
+		return errno;
 	/* Make it close if we go away. */
 	talloc_steal(trans, trans->tdb);
 
@@ -194,24 +188,22 @@ void do_transaction_start(struct connection *conn, struct buffered_data *in)
 
 	snprintf(id_str, sizeof(id_str), "%u", trans->id);
 	send_reply(conn, XS_TRANSACTION_START, id_str, strlen(id_str)+1);
+
+	return 0;
 }
 
-void do_transaction_end(struct connection *conn, struct buffered_data *in)
+int 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;
 
-	if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
+		return EINVAL;
 
-	if ((trans = conn->transaction) == NULL) {
-		send_error(conn, ENOENT);
-		return;
-	}
+	if ((trans = conn->transaction) == NULL)
+		return ENOENT;
 
 	conn->transaction = NULL;
 	list_del(&trans->list);
@@ -222,14 +214,10 @@ void do_transaction_end(struct connection *conn, struct buffered_data *in)
 
 	if (streq(arg, "T")) {
 		/* FIXME: Merge, rather failing on any change. */
-		if (trans->generation != generation) {
-			send_error(conn, EAGAIN);
-			return;
-		}
-		if (!replace_tdb(trans->tdb_name, trans->tdb)) {
-			send_error(conn, errno);
-			return;
-		}
+		if (trans->generation != generation)
+			return EAGAIN;
+		if (!replace_tdb(trans->tdb_name, trans->tdb))
+			return errno;
 		/* Don't close this: we won! */
 		trans->tdb = NULL;
 
@@ -243,6 +231,8 @@ void do_transaction_end(struct connection *conn, struct buffered_data *in)
 		generation += trans->trans_gen;
 	}
 	send_ack(conn, XS_TRANSACTION_END);
+
+	return 0;
 }
 
 void transaction_entry_inc(struct transaction *trans, unsigned int domid)
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index 7f0a781..aeeac3d 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -21,8 +21,8 @@
 
 struct transaction;
 
-void do_transaction_start(struct connection *conn, struct buffered_data *node);
-void do_transaction_end(struct connection *conn, struct buffered_data *in);
+int do_transaction_start(struct connection *conn, struct buffered_data *node);
+int do_transaction_end(struct connection *conn, struct buffered_data *in);
 
 struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 856750e..8cfc5b0 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -121,46 +121,36 @@ static int destroy_watch(void *_watch)
 	return 0;
 }
 
-void do_watch(struct connection *conn, struct buffered_data *in)
+int do_watch(struct connection *conn, struct buffered_data *in)
 {
 	struct watch *watch;
 	char *vec[2];
 	bool relative;
 
-	if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec))
+		return EINVAL;
 
 	if (strstarts(vec[0], "@")) {
 		relative = false;
-		if (strlen(vec[0]) > XENSTORE_REL_PATH_MAX) {
-			send_error(conn, EINVAL);
-			return;
-		}
+		if (strlen(vec[0]) > XENSTORE_REL_PATH_MAX)
+			return EINVAL;
 		/* check if valid event */
 	} else {
 		relative = !strstarts(vec[0], "/");
 		vec[0] = canonicalize(conn, vec[0]);
-		if (!is_valid_nodename(vec[0])) {
-			send_error(conn, EINVAL);
-			return;
-		}
+		if (!is_valid_nodename(vec[0]))
+			return EINVAL;
 	}
 
 	/* Check for duplicates. */
 	list_for_each_entry(watch, &conn->watches, list) {
 		if (streq(watch->node, vec[0]) &&
-		    streq(watch->token, vec[1])) {
-			send_error(conn, EEXIST);
-			return;
-		}
+		    streq(watch->token, vec[1]))
+			return EEXIST;
 	}
 
-	if (domain_watch(conn) > quota_nb_watch_per_domain) {
-		send_error(conn, E2BIG);
-		return;
-	}
+	if (domain_watch(conn) > quota_nb_watch_per_domain)
+		return E2BIG;
 
 	watch = talloc(conn, struct watch);
 	watch->node = talloc_strdup(watch, vec[0]);
@@ -180,17 +170,17 @@ void do_watch(struct connection *conn, struct buffered_data *in)
 
 	/* We fire once up front: simplifies clients and restart. */
 	add_event(conn, in, watch, watch->node);
+
+	return 0;
 }
 
-void do_unwatch(struct connection *conn, struct buffered_data *in)
+int do_unwatch(struct connection *conn, struct buffered_data *in)
 {
 	struct watch *watch;
 	char *node, *vec[2];
 
-	if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) {
-		send_error(conn, EINVAL);
-		return;
-	}
+	if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec))
+		return EINVAL;
 
 	node = canonicalize(conn, vec[0]);
 	list_for_each_entry(watch, &conn->watches, list) {
@@ -199,10 +189,10 @@ void do_unwatch(struct connection *conn, struct buffered_data *in)
 			talloc_free(watch);
 			domain_watch_dec(conn);
 			send_ack(conn, XS_UNWATCH);
-			return;
+			return 0;
 		}
 	}
-	send_error(conn, ENOENT);
+	return ENOENT;
 }
 
 void conn_delete_all_watches(struct connection *conn)
diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h
index 8ed1dde..546a5c3 100644
--- a/tools/xenstore/xenstored_watch.h
+++ b/tools/xenstore/xenstored_watch.h
@@ -21,8 +21,8 @@
 
 #include "xenstored_core.h"
 
-void do_watch(struct connection *conn, struct buffered_data *in);
-void do_unwatch(struct connection *conn, struct buffered_data *in);
+int do_watch(struct connection *conn, struct buffered_data *in);
+int 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, void *tmp, const char *name,
-- 
2.10.2


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

[PATCH v4 09/12] xenstore: make functions static

[Juergen Gross]

Move functions used in only one source to the file where they are used
and make them static.

Remove some prototypes from headers which are no longer in use.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: don't remove functions of libxenstore as requested by Andrew Cooper
---
 tools/xenstore/xenstored_core.c  |  55 ++++++-------------
 tools/xenstore/xenstored_core.h  |  14 -----
 tools/xenstore/xenstored_watch.c |  27 ++++++++++
 tools/xenstore/xenstored_watch.h |   2 -
 tools/xenstore/xs.c              | 111 ++++++++++++++++++++++++++++++++++++++
 tools/xenstore/xs_lib.c          | 112 ---------------------------------------
 6 files changed, 153 insertions(+), 168 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 938b652..438a8ce 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -365,22 +365,6 @@ static void initialize_fds(int sock, int *p_sock_pollfd_idx,
 	}
 }
 
-/* Is child a subnode of parent, or equal? */
-bool is_child(const char *child, const char *parent)
-{
-	unsigned int len = strlen(parent);
-
-	/* / should really be "" for this algorithm to work, but that's a
-	 * usability nightmare. */
-	if (streq(parent, "/"))
-		return true;
-
-	if (strncmp(child, parent, len) != 0)
-		return false;
-
-	return child[len] == '/' || child[len] == '\0';
-}
-
 /*
  * If it fails, returns NULL and sets errno.
  * Temporary memory allocations will be done with ctx.
@@ -638,6 +622,21 @@ unsigned int get_strings(struct buffered_data *data,
 	return i;
 }
 
+static void send_error(struct connection *conn, int error)
+{
+	unsigned int i;
+
+	for (i = 0; error != xsd_errors[i].errnum; i++) {
+		if (i == ARRAY_SIZE(xsd_errors) - 1) {
+			eprintf("xenstored: error %i untranslatable", error);
+			i = 0; /* EINVAL */
+			break;
+		}
+	}
+	send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
+			  strlen(xsd_errors[i].errstring) + 1);
+}
+
 void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 		const void *data, unsigned int len)
 {
@@ -675,21 +674,6 @@ void send_ack(struct connection *conn, enum xsd_sockmsg_type type)
 	send_reply(conn, type, "OK", sizeof("OK"));
 }
 
-void send_error(struct connection *conn, int error)
-{
-	unsigned int i;
-
-	for (i = 0; error != xsd_errors[i].errnum; i++) {
-		if (i == ARRAY_SIZE(xsd_errors) - 1) {
-			eprintf("xenstored: error %i untranslatable", error);
-			i = 0; 	/* EINVAL */
-			break;
-		}
-	}
-	send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
-			  strlen(xsd_errors[i].errstring) + 1);
-}
-
 static bool valid_chars(const char *node)
 {
 	/* Nodes can have lots of crap. */
@@ -761,15 +745,6 @@ char *canonicalize(struct connection *conn, const char *node)
 	return (char *)node;
 }
 
-bool check_event_node(const char *node)
-{
-	if (!node || !strstarts(node, "@")) {
-		errno = EINVAL;
-		return false;
-	}
-	return true;
-}
-
 static int send_directory(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 089625f..3872dab 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -132,24 +132,15 @@ const char *onearg(struct buffered_data *in);
 unsigned int get_strings(struct buffered_data *data,
 			 char *vec[], unsigned int num);
 
-/* Is child node a child or equal to parent node? */
-bool is_child(const char *child, const char *parent);
-
 void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 		const void *data, unsigned int len);
 
 /* Some routines (write, mkdir, etc) just need a non-error return */
 void send_ack(struct connection *conn, enum xsd_sockmsg_type type);
 
-/* Send an error: error is usually "errno". */
-void send_error(struct connection *conn, int error);
-
 /* Canonicalize this path if possible. */
 char *canonicalize(struct connection *conn, const char *node);
 
-/* Check if node is an event node. */
-bool check_event_node(const char *node);
-
 /* Get this node, checking we have permissions. */
 struct node *get_node(struct connection *conn,
 		      const void *ctx,
@@ -159,9 +150,6 @@ struct node *get_node(struct connection *conn,
 /* Get TDB context for this connection */
 TDB_CONTEXT *tdb_context(struct connection *conn);
 
-/* Destructor for tdbs: required for transaction code */
-int destroy_tdb(void *_tdb);
-
 /* Replace the tdb: required for transaction code */
 bool replace_tdb(const char *newname, TDB_CONTEXT *newtdb);
 
@@ -174,11 +162,9 @@ bool is_valid_nodename(const char *node);
 /* Tracing infrastructure. */
 void trace_create(const void *data, const char *type);
 void trace_destroy(const void *data, const char *type);
-void trace_watch_timeout(const struct connection *conn, const char *node, const char *token);
 void trace(const char *fmt, ...);
 void dtrace_io(const struct connection *conn, const struct buffered_data *data, int out);
 
-extern int event_fd;
 extern int dom0_domid;
 extern int dom0_event;
 extern int priv_domid;
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 8cfc5b0..e1146ed 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -47,6 +47,33 @@ struct watch
 	char *node;
 };
 
+static bool check_event_node(const char *node)
+{
+	if (!node || !strstarts(node, "@")) {
+		errno = EINVAL;
+		return false;
+	}
+	return true;
+}
+
+/* Is child a subnode of parent, or equal? */
+static bool is_child(const char *child, const char *parent)
+{
+	unsigned int len = strlen(parent);
+
+	/*
+	 * / should really be "" for this algorithm to work, but that's a
+	 * usability nightmare.
+	 */
+	if (streq(parent, "/"))
+		return true;
+
+	if (strncmp(child, parent, len) != 0)
+		return false;
+
+	return child[len] == '/' || child[len] == '\0';
+}
+
 /*
  * Send a watch event.
  * Temporary memory allocations are done with ctx.
diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h
index 546a5c3..c72ea6a 100644
--- a/tools/xenstore/xenstored_watch.h
+++ b/tools/xenstore/xenstored_watch.h
@@ -28,8 +28,6 @@ int do_unwatch(struct connection *conn, struct buffered_data *in);
 void fire_watches(struct connection *conn, void *tmp, const char *name,
 		  bool recurse);
 
-void dump_watches(struct connection *conn);
-
 void conn_delete_all_watches(struct connection *conn);
 
 #endif /* _XENSTORED_WATCH_H */
diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 40e3275..e462a20 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -1307,6 +1307,117 @@ static void *read_thread(void *arg)
 }
 #endif
 
+char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
+{
+	int want;
+	char *got;
+
+	if (ebuf->avail >= min_avail)
+		return ebuf->buf;
+
+	if (min_avail >= INT_MAX/3)
+		return 0;
+
+	want = ebuf->avail + min_avail + 10;
+	got = realloc(ebuf->buf, want);
+	if (!got)
+		return 0;
+
+	ebuf->buf = got;
+	ebuf->avail = want;
+	return ebuf->buf;
+}
+
+char *sanitise_value(struct expanding_buffer *ebuf,
+		     const char *val, unsigned len)
+{
+	int used, remain, c;
+	unsigned char *ip;
+
+#define ADD(c) (ebuf->buf[used++] = (c))
+#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
+
+	assert(len < INT_MAX/5);
+
+	ip = (unsigned char *)val;
+	used = 0;
+	remain = len;
+
+	if (!expanding_buffer_ensure(ebuf, remain + 1))
+		return NULL;
+
+	while (remain-- > 0) {
+		c= *ip++;
+
+		if (c >= ' ' && c <= '~' && c != '\\') {
+			ADD(c);
+			continue;
+		}
+
+		if (!expanding_buffer_ensure(ebuf, used + remain + 5))
+			/* for "<used>\\nnn<remain>\0" */
+			return 0;
+
+		ADD('\\');
+		switch (c) {
+		case '\t':  ADD('t');   break;
+		case '\n':  ADD('n');   break;
+		case '\r':  ADD('r');   break;
+		case '\\':  ADD('\\');  break;
+		default:
+			if (c < 010) ADDF("%03o", c);
+			else         ADDF("x%02x", c);
+		}
+	}
+
+	ADD(0);
+	assert(used <= ebuf->avail);
+	return ebuf->buf;
+
+#undef ADD
+#undef ADDF
+}
+
+void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
+{
+	const char *ip;
+	char *op;
+	unsigned c;
+	int n;
+
+	for (ip = in, op = out; (c = *ip++); *op++ = c) {
+		if (c == '\\') {
+			c = *ip++;
+
+#define GETF(f) do {					\
+			n = 0;				\
+			sscanf(ip, f "%n", &c, &n);	\
+			ip += n;			\
+		} while (0)
+
+			switch (c) {
+			case 't':              c= '\t';            break;
+			case 'n':              c= '\n';            break;
+			case 'r':              c= '\r';            break;
+			case '\\':             c= '\\';            break;
+			case 'x':                    GETF("%2x");  break;
+			case '0': case '4':
+			case '1': case '5':
+			case '2': case '6':
+			case '3': case '7':    --ip; GETF("%3o");  break;
+			case 0:                --ip;               break;
+			default:;
+			}
+#undef GETF
+		}
+	}
+
+	*op = 0;
+
+	if (out_len_r)
+		*out_len_r = op - out;
+}
+
 /*
  * Local variables:
  *  c-file-style: "linux"
diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index 6568e82..5ef3d6d 100644
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -21,7 +21,6 @@
 #include <string.h>
 #include <stdlib.h>
 #include <errno.h>
-#include <assert.h>
 #include "xenstore_lib.h"
 
 /* Common routines for the Xen store daemon and client library. */
@@ -184,114 +183,3 @@ unsigned int xs_count_strings(const char *strings, unsigned int len)
 
 	return num;
 }
-
-char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
-{
-	int want;
-	char *got;
-
-	if (ebuf->avail >= min_avail)
-		return ebuf->buf;
-
-	if (min_avail >= INT_MAX/3)
-		return 0;
-
-	want = ebuf->avail + min_avail + 10;
-	got = realloc(ebuf->buf, want);
-	if (!got)
-		return 0;
-
-	ebuf->buf = got;
-	ebuf->avail = want;
-	return ebuf->buf;
-}
-
-char *sanitise_value(struct expanding_buffer *ebuf,
-		     const char *val, unsigned len)
-{
-	int used, remain, c;
-	unsigned char *ip;
-
-#define ADD(c) (ebuf->buf[used++] = (c))
-#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
-
-	assert(len < INT_MAX/5);
-
-	ip = (unsigned char *)val;
-	used = 0;
-	remain = len;
-
-	if (!expanding_buffer_ensure(ebuf, remain + 1))
-		return NULL;
-
-	while (remain-- > 0) {
-		c= *ip++;
-
-		if (c >= ' ' && c <= '~' && c != '\\') {
-			ADD(c);
-			continue;
-		}
-
-		if (!expanding_buffer_ensure(ebuf, used + remain + 5))
-			/* for "<used>\\nnn<remain>\0" */
-			return 0;
-
-		ADD('\\');
-		switch (c) {
-		case '\t':  ADD('t');   break;
-		case '\n':  ADD('n');   break;
-		case '\r':  ADD('r');   break;
-		case '\\':  ADD('\\');  break;
-		default:
-			if (c < 010) ADDF("%03o", c);
-			else         ADDF("x%02x", c);
-		}
-	}
-
-	ADD(0);
-	assert(used <= ebuf->avail);
-	return ebuf->buf;
-
-#undef ADD
-#undef ADDF
-}
-
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
-{
-	const char *ip;
-	char *op;
-	unsigned c;
-	int n;
-
-	for (ip = in, op = out; (c = *ip++); *op++ = c) {
-		if (c == '\\') {
-			c = *ip++;
-
-#define GETF(f) do {					\
-		        n = 0;				\
-                        sscanf(ip, f "%n", &c, &n);	\
-			ip += n;			\
-		} while (0)
-
-			switch (c) {
-			case 't':              c= '\t';            break;
-			case 'n':              c= '\n';            break;
-			case 'r':              c= '\r';            break;
-			case '\\':             c= '\\';            break;
-			case 'x':                    GETF("%2x");  break;
-			case '0': case '4':
-			case '1': case '5':
-			case '2': case '6':
-			case '3': case '7':    --ip; GETF("%3o");  break;
-			case 0:                --ip;               break;
-			default:;
-			}
-#undef GETF
-		}
-	}
-
-	*op = 0;
-
-	if (out_len_r)
-		*out_len_r = op - out;
-}
-- 
2.10.2


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

Re: [PATCH v4 09/12] xenstore: make functions static

[Wei Liu]

On Mon, Dec 05, 2016 at 08:48:50AM +0100, Juergen Gross wrote:
> Move functions used in only one source to the file where they are used
> and make them static.
> 
> Remove some prototypes from headers which are no longer in use.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v4 10/12] xenstore: add helper functions for wire argument parsing

[Juergen Gross]

The xenstore wire command argument parsing of the different commands
is repeating some patterns multiple times. Add some helper functions
to avoid the duplicated code.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c   | 66 ++++++++++++++--------------
 tools/xenstore/xenstored_domain.c | 90 +++++++++++++++++++--------------------
 2 files changed, 77 insertions(+), 79 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 438a8ce..a118458 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -745,13 +745,25 @@ char *canonicalize(struct connection *conn, const char *node)
 	return (char *)node;
 }
 
+static struct node *get_node_canonicalized(struct connection *conn,
+					   const void *ctx,
+					   const char *name,
+					   char **canonical_name,
+					   enum xs_perm_type perm)
+{
+	char *tmp_name;
+
+	if (!canonical_name)
+		canonical_name = &tmp_name;
+	*canonical_name = canonicalize(conn, name);
+	return get_node(conn, ctx, *canonical_name, perm);
+}
+
 static int 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, in, name, XS_PERM_READ);
+	node = get_node_canonicalized(conn, in, onearg(in), NULL, XS_PERM_READ);
 	if (!node)
 		return errno;
 
@@ -764,7 +776,7 @@ static int send_directory_part(struct connection *conn,
 			       struct buffered_data *in)
 {
 	unsigned int off, len, maxlen, genlen;
-	char *name, *child, *data;
+	char *child, *data;
 	struct node *node;
 	char gen[24];
 
@@ -772,15 +784,13 @@ static int send_directory_part(struct connection *conn,
 		return EINVAL;
 
 	/* First arg is node name. */
-	name = canonicalize(conn, in->buffer);
+	node = get_node_canonicalized(conn, in, in->buffer, NULL, XS_PERM_READ);
+	if (!node)
+		return errno;
 
 	/* Second arg is childlist offset. */
 	off = atoi(in->buffer + strlen(in->buffer) + 1);
 
-	node = get_node(conn, in, name, XS_PERM_READ);
-	if (!node)
-		return errno;
-
 	genlen = snprintf(gen, sizeof(gen), "%"PRIu64, node->generation) + 1;
 
 	/* Offset behind list: just return a list with an empty string. */
@@ -820,10 +830,8 @@ static int send_directory_part(struct connection *conn,
 static int 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, in, name, XS_PERM_READ);
+	node = get_node_canonicalized(conn, in, onearg(in), NULL, XS_PERM_READ);
 	if (!node)
 		return errno;
 
@@ -962,8 +970,7 @@ static int do_write(struct connection *conn, struct buffered_data *in)
 	offset = strlen(vec[0]) + 1;
 	datalen = in->used - offset;
 
-	name = canonicalize(conn, vec[0]);
-	node = get_node(conn, in, name, XS_PERM_WRITE);
+	node = get_node_canonicalized(conn, in, vec[0], &name, XS_PERM_WRITE);
 	if (!node) {
 		/* No permissions, invalid input? */
 		if (errno != ENOENT)
@@ -987,13 +994,10 @@ static int do_write(struct connection *conn, struct buffered_data *in)
 static int do_mkdir(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
-	const char *name = onearg(in);
+	char *name;
 
-	if (!name)
-		return EINVAL;
-
-	name = canonicalize(conn, name);
-	node = get_node(conn, in, name, XS_PERM_WRITE);
+	node = get_node_canonicalized(conn, in, onearg(in), &name,
+				      XS_PERM_WRITE);
 
 	/* If it already exists, fine. */
 	if (!node) {
@@ -1103,10 +1107,10 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
 	int ret;
-	const char *name = onearg(in);
+	char *name;
 
-	name = canonicalize(conn, name);
-	node = get_node(conn, in, name, XS_PERM_WRITE);
+	node = get_node_canonicalized(conn, in, onearg(in), &name,
+				      XS_PERM_WRITE);
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
@@ -1138,12 +1142,10 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 static int do_get_perms(struct connection *conn, struct buffered_data *in)
 {
 	struct node *node;
-	const char *name = onearg(in);
 	char *strings;
 	unsigned int len;
 
-	name = canonicalize(conn, name);
-	node = get_node(conn, in, name, XS_PERM_READ);
+	node = get_node_canonicalized(conn, in, onearg(in), NULL, XS_PERM_READ);
 	if (!node)
 		return errno;
 
@@ -1168,15 +1170,15 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
 		return EINVAL;
 
 	/* First arg is node name. */
-	name = canonicalize(conn, in->buffer);
+	/* We must own node to do this (tools can do this too). */
+	node = get_node_canonicalized(conn, in, in->buffer, &name,
+				      XS_PERM_WRITE | XS_PERM_OWNER);
+	if (!node)
+		return errno;
+
 	permstr = in->buffer + strlen(in->buffer) + 1;
 	num--;
 
-	/* We must own node to do this (tools can do this too). */
-	node = get_node(conn, in, name, XS_PERM_WRITE|XS_PERM_OWNER);
-	if (!node)
-		return errno;
-
 	perms = talloc_array(node, struct xs_permissions, num);
 	if (!xs_strings_to_perms(perms, num, permstr))
 		return errno;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 0b2af13..2443b08 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -399,6 +399,18 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
 	return 0;
 }
 
+static struct domain *find_connected_domain(unsigned int domid)
+{
+	struct domain *domain;
+
+	domain = find_domain_by_domid(domid);
+	if (!domain)
+		return ERR_PTR(-ENOENT);
+	if (!domain->conn)
+		return ERR_PTR(-EINVAL);
+	return domain;
+}
+
 int do_set_target(struct connection *conn, struct buffered_data *in)
 {
 	char *vec[2];
@@ -413,18 +425,13 @@ int do_set_target(struct connection *conn, struct buffered_data *in)
 	domid = atoi(vec[0]);
 	tdomid = atoi(vec[1]);
 
-        domain = find_domain_by_domid(domid);
-	if (!domain)
-		return ENOENT;
-        if (!domain->conn)
-		return EINVAL;
+        domain = find_connected_domain(domid);
+	if (IS_ERR(domain))
+		return -PTR_ERR(domain);
 
-        tdomain = find_domain_by_domid(tdomid);
-	if (!tdomain)
-		return ENOENT;
-
-        if (!tdomain->conn)
-		return EINVAL;
+        tdomain = find_connected_domain(tdomid);
+	if (IS_ERR(tdomain))
+		return -PTR_ERR(tdomain);
 
         talloc_reference(domain->conn, tdomain->conn);
         domain->conn->target = tdomain->conn;
@@ -434,29 +441,33 @@ int do_set_target(struct connection *conn, struct buffered_data *in)
 	return 0;
 }
 
+static struct domain *onearg_domain(struct connection *conn,
+				    struct buffered_data *in)
+{
+	const char *domid_str = onearg(in);
+	unsigned int domid;
+
+	if (!domid_str)
+		return ERR_PTR(-EINVAL);
+
+	domid = atoi(domid_str);
+	if (!domid)
+		return ERR_PTR(-EINVAL);
+
+	if (domain_is_unprivileged(conn))
+		return ERR_PTR(-EACCES);
+
+	return find_connected_domain(domid);
+}
+
 /* domid */
 int do_release(struct connection *conn, struct buffered_data *in)
 {
-	const char *domid_str = onearg(in);
 	struct domain *domain;
-	unsigned int domid;
 
-	if (!domid_str)
-		return EINVAL;
-
-	domid = atoi(domid_str);
-	if (!domid)
-		return EINVAL;
-
-	if (domain_is_unprivileged(conn))
-		return EACCES;
-
-	domain = find_domain_by_domid(domid);
-	if (!domain)
-		return ENOENT;
-
-	if (!domain->conn)
-		return EINVAL;
+	domain = onearg_domain(conn, in);
+	if (IS_ERR(domain))
+		return -PTR_ERR(domain);
 
 	talloc_free(domain->conn);
 
@@ -468,25 +479,10 @@ int do_release(struct connection *conn, struct buffered_data *in)
 int do_resume(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
-	unsigned int domid;
-	const char *domid_str = onearg(in);
 
-	if (!domid_str)
-		return EINVAL;
-
-	domid = atoi(domid_str);
-	if (!domid)
-		return EINVAL;
-
-	if (domain_is_unprivileged(conn))
-		return EACCES;
-
-	domain = find_domain_by_domid(domid);
-	if (!domain)
-		return ENOENT;
-
-	if (!domain->conn)
-		return EINVAL;
+	domain = onearg_domain(conn, in);
+	if (IS_ERR(domain))
+		return -PTR_ERR(domain);
 
 	domain->shutdown = 0;
 	
-- 
2.10.2


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

[PATCH v4 11/12] xenstore: add small default data buffer to internal struct

[Juergen Gross]

Instead of always allocating a data buffer for incoming or outgoing
xenstore wire data add a small buffer to the buffered_data structure
of xenstored. This has the advantage that especially sending simple
response messages like errors or "OK" will no longer need allocating
a data buffer. This requires adding a memory context where the
allocated buffer was used for that purpose.

In order to avoid allocating a new buffered_data structure for each
response reuse the structure of the original request. This in turn
will avoid any new memory allocations for sending e.g. an ENOMEM
response making it possible to send it at all. To do this the
allocation of the buffered_data structure for the incoming request
must be done when a new request is recognized instead of doing it
when accepting a new connect.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c        | 80 +++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |  6 ++-
 tools/xenstore/xenstored_domain.c      |  4 +-
 tools/xenstore/xenstored_transaction.c |  4 +-
 tools/xenstore/xenstored_watch.c       |  4 +-
 5 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index a118458..670aed9 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -647,17 +647,20 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 		return;
 	}
 
-	/* Message is a child of the connection context for auto-cleanup. */
-	bdata = new_buffer(conn);
-	bdata->buffer = talloc_array(bdata, char, len);
-
-	/* Echo request header in reply unless this is an async watch event. */
+	/* Replies reuse the request buffer, events need a new one. */
 	if (type != XS_WATCH_EVENT) {
-		memcpy(&bdata->hdr.msg, &conn->in->hdr.msg,
-		       sizeof(struct xsd_sockmsg));
+		bdata = conn->in;
+		bdata->inhdr = true;
+		bdata->used = 0;
+		conn->in = NULL;
 	} else {
-		memset(&bdata->hdr.msg, 0, sizeof(struct xsd_sockmsg));
+		/* Message is a child of the connection for auto-cleanup. */
+		bdata = new_buffer(conn);
 	}
+	if (len <= DEFAULT_BUFFER_SIZE)
+		bdata->buffer = bdata->default_buffer;
+	else
+		bdata->buffer = talloc_array(bdata, char, len);
 
 	/* Update relevant header fields and fill in the message body. */
 	bdata->hdr.msg.type = type;
@@ -733,7 +736,7 @@ static char *perms_to_strings(const void *ctx,
 	return strings;
 }
 
-char *canonicalize(struct connection *conn, const char *node)
+char *canonicalize(struct connection *conn, const void *ctx, const char *node)
 {
 	const char *prefix;
 
@@ -741,7 +744,7 @@ char *canonicalize(struct connection *conn, const char *node)
 		return (char *)node;
 	prefix = get_implicit_path(conn);
 	if (prefix)
-		return talloc_asprintf(node, "%s/%s", prefix, node);
+		return talloc_asprintf(ctx, "%s/%s", prefix, node);
 	return (char *)node;
 }
 
@@ -755,7 +758,7 @@ static struct node *get_node_canonicalized(struct connection *conn,
 
 	if (!canonical_name)
 		canonical_name = &tmp_name;
-	*canonical_name = canonicalize(conn, name);
+	*canonical_name = canonicalize(conn, ctx, name);
 	return get_node(conn, ctx, *canonical_name, perm);
 }
 
@@ -865,17 +868,18 @@ static char *basename(const char *name)
 	return strrchr(name, '/') + 1;
 }
 
-static struct node *construct_node(struct connection *conn, const char *name)
+static struct node *construct_node(struct connection *conn, const void *ctx,
+				   const char *name)
 {
 	const char *base;
 	unsigned int baselen;
 	struct node *parent, *node;
-	char *children, *parentname = get_parent(name, name);
+	char *children, *parentname = get_parent(ctx, name);
 
 	/* If parent doesn't exist, create it. */
 	parent = read_node(conn, parentname, parentname);
 	if (!parent)
-		parent = construct_node(conn, parentname);
+		parent = construct_node(conn, ctx, parentname);
 	if (!parent)
 		return NULL;
 
@@ -885,14 +889,14 @@ static struct node *construct_node(struct connection *conn, const char *name)
 	/* Add child to parent. */
 	base = basename(name);
 	baselen = strlen(base) + 1;
-	children = talloc_array(name, char, parent->childlen + baselen);
+	children = talloc_array(ctx, char, parent->childlen + baselen);
 	memcpy(children, parent->children, parent->childlen);
 	memcpy(children + parent->childlen, base, baselen);
 	parent->children = children;
 	parent->childlen += baselen;
 
 	/* Allocate node */
-	node = talloc(name, struct node);
+	node = talloc(ctx, struct node);
 	node->tdb = tdb_context(conn);
 	node->name = talloc_strdup(node, name);
 
@@ -926,13 +930,13 @@ static int destroy_node(void *_node)
 	return 0;
 }
 
-static struct node *create_node(struct connection *conn, 
+static struct node *create_node(struct connection *conn, const void *ctx,
 				const char *name,
 				void *data, unsigned int datalen)
 {
 	struct node *node, *i;
 
-	node = construct_node(conn, name);
+	node = construct_node(conn, ctx, name);
 	if (!node)
 		return NULL;
 
@@ -975,7 +979,8 @@ static int do_write(struct connection *conn, struct buffered_data *in)
 		/* No permissions, invalid input? */
 		if (errno != ENOENT)
 			return errno;
-		node = create_node(conn, name, in->buffer + offset, datalen);
+		node = create_node(conn, in, name, in->buffer + offset,
+				   datalen);
 		if (!node)
 			return errno;
 	} else {
@@ -1004,7 +1009,7 @@ static int do_mkdir(struct connection *conn, struct buffered_data *in)
 		/* No permissions? */
 		if (errno != ENOENT)
 			return errno;
-		node = create_node(conn, name, NULL, 0);
+		node = create_node(conn, in, name, NULL, 0);
 		if (!node)
 			return errno;
 		fire_watches(conn, in, name, false);
@@ -1075,12 +1080,13 @@ static bool delete_child(struct connection *conn,
 }
 
 
-static int _rm(struct connection *conn, struct node *node, const char *name)
+static int _rm(struct connection *conn, const void *ctx, 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, name, get_parent(name, name));
+	struct node *parent = read_node(conn, ctx, get_parent(ctx, name));
 	if (!parent)
 		return EINVAL;
 
@@ -1097,7 +1103,7 @@ static void internal_rm(const char *name)
 	char *tname = talloc_strdup(NULL, name);
 	struct node *node = read_node(NULL, tname, tname);
 	if (node)
-		_rm(NULL, node, tname);
+		_rm(NULL, tname, node, tname);
 	talloc_free(node);
 	talloc_free(tname);
 }
@@ -1128,7 +1134,7 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 	if (streq(name, "/"))
 		return EINVAL;
 
-	ret = _rm(conn, node, name);
+	ret = _rm(conn, in, node, name);
 	if (ret)
 		return ret;
 
@@ -1301,8 +1307,7 @@ static void consider_message(struct connection *conn)
 
 	process_message(conn, conn->in);
 
-	talloc_free(conn->in);
-	conn->in = new_buffer(conn);
+	assert(conn->in == NULL);
 }
 
 /* Errors in reading or allocating here mean we get out of sync, so we
@@ -1310,7 +1315,15 @@ static void consider_message(struct connection *conn)
 static void handle_input(struct connection *conn)
 {
 	int bytes;
-	struct buffered_data *in = conn->in;
+	struct buffered_data *in;
+
+	if (!conn->in) {
+		conn->in = new_buffer(conn);
+		/* In case of no memory just try it next time again. */
+		if (!conn->in)
+			return;
+	}
+	in = conn->in;
 
 	/* Not finished header yet? */
 	if (in->inhdr) {
@@ -1328,7 +1341,10 @@ static void handle_input(struct connection *conn)
 			goto bad_client;
 		}
 
-		in->buffer = talloc_array(in, char, in->hdr.msg.len);
+		if (in->hdr.msg.len <= DEFAULT_BUFFER_SIZE)
+			in->buffer = in->default_buffer;
+		else
+			in->buffer = talloc_array(in, char, in->hdr.msg.len);
 		if (!in->buffer)
 			goto bad_client;
 		in->used = 0;
@@ -1377,12 +1393,6 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
 	INIT_LIST_HEAD(&new->watches);
 	INIT_LIST_HEAD(&new->transaction_list);
 
-	new->in = new_buffer(new);
-	if (new->in == NULL) {
-		talloc_free(new);
-		return NULL;
-	}
-
 	list_add_tail(&new->list, &connections);
 	talloc_set_destructor(new, destroy_conn);
 	trace_create(new, "connection");
@@ -1522,7 +1532,7 @@ static void setup_structure(void)
 
 		if (remove_local) {
 			internal_rm("/local");
-			create_node(NULL, tlocal, NULL, 0);
+			create_node(NULL, NULL, tlocal, NULL, 0);
 
 			check_store();
 		}
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3872dab..f6a56f7 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -33,6 +33,9 @@
 #include "list.h"
 #include "tdb.h"
 
+/* DEFAULT_BUFFER_SIZE should be large enough for each errno string. */
+#define DEFAULT_BUFFER_SIZE 16
+
 struct buffered_data
 {
 	struct list_head list;
@@ -50,6 +53,7 @@ struct buffered_data
 
 	/* The actual data. */
 	char *buffer;
+	char default_buffer[DEFAULT_BUFFER_SIZE];
 };
 
 struct connection;
@@ -139,7 +143,7 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 void send_ack(struct connection *conn, enum xsd_sockmsg_type type);
 
 /* Canonicalize this path if possible. */
-char *canonicalize(struct connection *conn, const char *node);
+char *canonicalize(struct connection *conn, const void *ctx, const char *node);
 
 /* Get this node, checking we have permissions. */
 struct node *get_node(struct connection *conn,
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 2443b08..fefed5e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -329,9 +329,7 @@ static void domain_conn_reset(struct domain *domain)
 		talloc_free(out);
 	}
 
-	talloc_free(conn->in->buffer);
-	memset(conn->in, 0, sizeof(*conn->in));
-	conn->in->inhdr = true;
+	talloc_free(conn->in);
 
 	domain->interface->req_cons = domain->interface->req_prod = 0;
 	domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 423fd3e..d314cd5 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -209,8 +209,8 @@ int do_transaction_end(struct connection *conn, struct buffered_data *in)
 	list_del(&trans->list);
 	conn->transaction_started--;
 
-	/* Attach transaction to arg for auto-cleanup */
-	talloc_steal(arg, trans);
+	/* Attach transaction to in for auto-cleanup */
+	talloc_steal(in, trans);
 
 	if (streq(arg, "T")) {
 		/* FIXME: Merge, rather failing on any change. */
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index e1146ed..94251db 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -164,7 +164,7 @@ int do_watch(struct connection *conn, struct buffered_data *in)
 		/* check if valid event */
 	} else {
 		relative = !strstarts(vec[0], "/");
-		vec[0] = canonicalize(conn, vec[0]);
+		vec[0] = canonicalize(conn, in, vec[0]);
 		if (!is_valid_nodename(vec[0]))
 			return EINVAL;
 	}
@@ -209,7 +209,7 @@ int do_unwatch(struct connection *conn, struct buffered_data *in)
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec))
 		return EINVAL;
 
-	node = canonicalize(conn, vec[0]);
+	node = canonicalize(conn, in, vec[0]);
 	list_for_each_entry(watch, &conn->watches, list) {
 		if (streq(watch->node, node) && streq(watch->token, vec[1])) {
 			list_del(&watch->list);
-- 
2.10.2


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

[PATCH v4 12/12] xenstore: handle memory allocation failures in xenstored

[Juergen Gross]

Check for failures when allocating new memory in xenstored.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: add alloc failure check in delete_node() and add_change_node()
    clean_store() only if no failure in check_store_()
---
 tools/xenstore/xenstored_core.c        | 210 +++++++++++++++++++++++++--------
 tools/xenstore/xenstored_domain.c      |  10 ++
 tools/xenstore/xenstored_transaction.c |  26 ++++
 tools/xenstore/xenstored_watch.c       |  12 ++
 4 files changed, 211 insertions(+), 47 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 670aed9..cf6c2da 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -149,8 +149,10 @@ void trace(const char *fmt, ...)
 	va_start(arglist, fmt);
 	str = talloc_vasprintf(NULL, fmt, arglist);
 	va_end(arglist);
-	dummy = write(tracefd, str, strlen(str));
-	talloc_free(str);
+	if (str) {
+		dummy = write(tracefd, str, strlen(str));
+		talloc_free(str);
+	}
 }
 
 static void trace_io(const struct connection *conn,
@@ -392,7 +394,16 @@ static struct node *read_node(struct connection *conn, const void *ctx,
 	}
 
 	node = talloc(ctx, struct node);
+	if (!node) {
+		errno = ENOMEM;
+		return NULL;
+	}
 	node->name = talloc_strdup(node, name);
+	if (!node->name) {
+		talloc_free(node);
+		errno = ENOMEM;
+		return NULL;
+	}
 	node->parent = NULL;
 	node->tdb = tdb_context(conn);
 	talloc_steal(node, data.dptr);
@@ -490,35 +501,46 @@ static enum xs_perm_type perm_for_conn(struct connection *conn,
  */
 static char *get_parent(const void *ctx, const char *node)
 {
+	char *parent;
 	char *slash = strrchr(node + 1, '/');
-	if (!slash)
-		return talloc_strdup(ctx, "/");
-	return talloc_asprintf(ctx, "%.*s", (int)(slash - node), node);
+
+	parent = slash ? talloc_asprintf(ctx, "%.*s", (int)(slash - node), node)
+		       : talloc_strdup(ctx, "/");
+	if (!parent)
+		errno = ENOMEM;
+
+	return parent;
 }
 
 /*
  * What do parents say?
  * Temporary memory allocations are done with ctx.
  */
-static enum xs_perm_type ask_parents(struct connection *conn, const void *ctx,
-				     const char *name)
+static int ask_parents(struct connection *conn, const void *ctx,
+		       const char *name, enum xs_perm_type *perm)
 {
 	struct node *node;
 
 	do {
 		name = get_parent(ctx, name);
+		if (!name)
+			return errno;
 		node = read_node(conn, ctx, name);
 		if (node)
 			break;
+		if (errno == ENOMEM)
+			return errno;
 	} while (!streq(name, "/"));
 
 	/* No permission at root?  We're in trouble. */
 	if (!node) {
 		corrupt(conn, "No permissions file at root");
-		return XS_PERM_NONE;
+		*perm = XS_PERM_NONE;
+		return 0;
 	}
 
-	return perm_for_conn(conn, node->perms, node->num_perms);
+	*perm = perm_for_conn(conn, node->perms, node->num_perms);
+	return 0;
 }
 
 /*
@@ -532,11 +554,15 @@ static int errno_from_parents(struct connection *conn, const void *ctx,
 			      const char *node, int errnum,
 			      enum xs_perm_type perm)
 {
+	enum xs_perm_type parent_perm = XS_PERM_NONE;
+
 	/* We always tell them about memory failures. */
 	if (errnum == ENOMEM)
 		return errnum;
 
-	if (ask_parents(conn, ctx, node) & perm)
+	if (ask_parents(conn, ctx, node, &parent_perm))
+		return errno;
+	if (parent_perm & perm)
 		return errnum;
 	return EACCES;
 }
@@ -566,7 +592,7 @@ struct node *get_node(struct connection *conn,
 		}
 	}
 	/* Clean up errno if they weren't supposed to know. */
-	if (!node) 
+	if (!node && errno != ENOMEM)
 		errno = errno_from_parents(conn, ctx, name, errno, perm);
 	return node;
 }
@@ -656,11 +682,29 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 	} else {
 		/* Message is a child of the connection for auto-cleanup. */
 		bdata = new_buffer(conn);
+
+		/*
+		 * Allocation failure here is unfortunate: we have no way to
+		 * tell anybody about it.
+		 */
+		if (!bdata)
+			return;
 	}
 	if (len <= DEFAULT_BUFFER_SIZE)
 		bdata->buffer = bdata->default_buffer;
 	else
 		bdata->buffer = talloc_array(bdata, char, len);
+	if (!bdata->buffer) {
+		if (type == XS_WATCH_EVENT) {
+			/* Same as above: no way to tell someone. */
+			talloc_free(bdata);
+			return;
+		}
+		/* re-establish request buffer for sending ENOMEM. */
+		conn->in = bdata;
+		send_error(conn, ENOMEM);
+		return;
+	}
 
 	/* Update relevant header fields and fill in the message body. */
 	bdata->hdr.msg.type = type;
@@ -669,6 +713,8 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 
 	/* Queue for later transmission. */
 	list_add_tail(&bdata->list, &conn->out_list);
+
+	return;
 }
 
 /* Some routines (write, mkdir, etc) just need a non-error return */
@@ -730,6 +776,8 @@ static char *perms_to_strings(const void *ctx,
 
 		strings = talloc_realloc(ctx, strings, char,
 					 *len + strlen(buffer) + 1);
+		if (!strings)
+			return NULL;
 		strcpy(strings + *len, buffer);
 		*len += strlen(buffer) + 1;
 	}
@@ -876,6 +924,9 @@ static struct node *construct_node(struct connection *conn, const void *ctx,
 	struct node *parent, *node;
 	char *children, *parentname = get_parent(ctx, name);
 
+	if (!parentname)
+		return NULL;
+
 	/* If parent doesn't exist, create it. */
 	parent = read_node(conn, parentname, parentname);
 	if (!parent)
@@ -1023,6 +1074,7 @@ static void delete_node(struct connection *conn, struct node *node,
 			bool changed)
 {
 	unsigned int i;
+	char *name;
 
 	/* Delete self, then delete children.  If we crash, then the worst
 	   that can happen is the children will continue to take up space, but
@@ -1033,17 +1085,18 @@ 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, node,
-				  talloc_asprintf(node, "%s/%s", node->name,
-						  node->children + i));
+		name = talloc_asprintf(node, "%s/%s", node->name,
+				       node->children + i);
+		child = name ? read_node(conn, node, name) : NULL;
 		if (child) {
 			delete_node(conn, child, false);
 		}
 		else {
-			trace("delete_node: No child '%s/%s' found!\n",
+			trace("delete_node: Error deleting child '%s/%s'!\n",
 			      node->name, node->children + i);
 			/* Skip it, we've already deleted the parent. */
 		}
+		talloc_free(name);
 	}
 }
 
@@ -1086,9 +1139,15 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node,
 	/* 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, ctx, get_parent(ctx, name));
+	struct node *parent;
+	char *parentname = get_parent(ctx, name);
+
+	if (!parentname)
+		return errno;
+
+	parent = read_node(conn, ctx, parentname);
 	if (!parent)
-		return EINVAL;
+		return (errno == ENOMEM) ? ENOMEM : EINVAL;
 
 	if (!delete_child(conn, parent, basename(name)))
 		return EINVAL;
@@ -1114,19 +1173,24 @@ static int do_rm(struct connection *conn, struct buffered_data *in)
 	struct node *node;
 	int ret;
 	char *name;
+	char *parentname;
 
 	node = get_node_canonicalized(conn, in, onearg(in), &name,
 				      XS_PERM_WRITE);
 	if (!node) {
 		/* Didn't exist already?  Fine, if parent exists. */
 		if (errno == ENOENT) {
-			node = read_node(conn, in, get_parent(in, name));
+			parentname = get_parent(in, name);
+			if (!parentname)
+				return errno;
+			node = read_node(conn, in, parentname);
 			if (node) {
 				send_ack(conn, XS_RM);
 				return 0;
 			}
 			/* Restore errno, just in case. */
-			errno = ENOENT;
+			if (errno != ENOMEM)
+				errno = ENOENT;
 		}
 		return errno;
 	}
@@ -1186,6 +1250,8 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
 	num--;
 
 	perms = talloc_array(node, struct xs_permissions, num);
+	if (!perms)
+		return ENOMEM;
 	if (!xs_strings_to_perms(perms, num, permstr))
 		return errno;
 
@@ -1319,7 +1385,7 @@ static void handle_input(struct connection *conn)
 
 	if (!conn->in) {
 		conn->in = new_buffer(conn);
-		/* In case of no memory just try it next time again. */
+		/* In case of no memory just try it again next time. */
 		if (!conn->in)
 			return;
 	}
@@ -1327,26 +1393,29 @@ static void handle_input(struct connection *conn)
 
 	/* Not finished header yet? */
 	if (in->inhdr) {
-		bytes = conn->read(conn, in->hdr.raw + in->used,
-				   sizeof(in->hdr) - in->used);
-		if (bytes < 0)
-			goto bad_client;
-		in->used += bytes;
-		if (in->used != sizeof(in->hdr))
-			return;
+		if (in->used != sizeof(in->hdr)) {
+			bytes = conn->read(conn, in->hdr.raw + in->used,
+					   sizeof(in->hdr) - in->used);
+			if (bytes < 0)
+				goto bad_client;
+			in->used += bytes;
+			if (in->used != sizeof(in->hdr))
+				return;
 
-		if (in->hdr.msg.len > XENSTORE_PAYLOAD_MAX) {
-			syslog(LOG_ERR, "Client tried to feed us %i",
-			       in->hdr.msg.len);
-			goto bad_client;
+			if (in->hdr.msg.len > XENSTORE_PAYLOAD_MAX) {
+				syslog(LOG_ERR, "Client tried to feed us %i",
+				       in->hdr.msg.len);
+				goto bad_client;
+			}
 		}
 
 		if (in->hdr.msg.len <= DEFAULT_BUFFER_SIZE)
 			in->buffer = in->default_buffer;
 		else
 			in->buffer = talloc_array(in, char, in->hdr.msg.len);
+		/* In case of no memory just try it again next time. */
 		if (!in->buffer)
-			goto bad_client;
+			return;
 		in->used = 0;
 		in->inhdr = false;
 	}
@@ -1469,6 +1538,9 @@ static void manual_node(const char *name, const char *child)
 	struct xs_permissions perms = { .id = 0, .perms = XS_PERM_NONE };
 
 	node = talloc_zero(NULL, struct node);
+	if (!node)
+		barf_perror("Could not allocate initial node %s", name);
+
 	node->name = name;
 	node->perms = &perms;
 	node->num_perms = 1;
@@ -1506,6 +1578,8 @@ static void setup_structure(void)
 {
 	char *tdbname;
 	tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
+	if (!tdbname)
+		barf_perror("Could not create tdbname");
 
 	if (!(tdb_flags & TDB_INTERNAL))
 		tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
@@ -1584,11 +1658,14 @@ static char *child_name(const char *s1, const char *s2)
 }
 
 
-static void remember_string(struct hashtable *hash, const char *str)
+static int remember_string(struct hashtable *hash, const char *str)
 {
 	char *k = malloc(strlen(str) + 1);
+
+	if (!k)
+		return 0;
 	strcpy(k, str);
-	hashtable_insert(hash, k, (void *)1);
+	return hashtable_insert(hash, k, (void *)1);
 }
 
 
@@ -1605,9 +1682,10 @@ static void remember_string(struct hashtable *hash, const char *str)
  * As we go, we record each node in the given reachable hashtable.  These
  * entries will be used later in clean_store.
  */
-static void check_store_(const char *name, struct hashtable *reachable)
+static int check_store_(const char *name, struct hashtable *reachable)
 {
 	struct node *node = read_node(NULL, name, name);
+	int ret = 0;
 
 	if (node) {
 		size_t i = 0;
@@ -1615,14 +1693,24 @@ static void check_store_(const char *name, struct hashtable *reachable)
 		struct hashtable * children =
 			create_hashtable(16, hash_from_key_fn, keys_equal_fn);
 
-		remember_string(reachable, name);
+		if (!remember_string(reachable, name)) {
+			hashtable_destroy(children, 0);
+			log("check_store: ENOMEM");
+			return ENOMEM;
+		}
 
-		while (i < node->childlen) {
+		while (i < node->childlen && !ret) {
+			struct node *childnode;
 			size_t childlen = strlen(node->children + i);
 			char * childname = child_name(node->name,
 						      node->children + i);
-			struct node *childnode = read_node(NULL, childname,
-							   childname);
+
+			if (!childname) {
+				log("check_store: ENOMEM");
+				ret = ENOMEM;
+				break;
+			}
+			childnode = read_node(NULL, childname, childname);
 			
 			if (childnode) {
 				if (hashtable_search(children, childname)) {
@@ -1636,11 +1724,18 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					}
 				}
 				else {
-					remember_string(children, childname);
-					check_store_(childname, reachable);
+					if (!remember_string(children,
+							     childname)) {
+						log("check_store: ENOMEM");
+						talloc_free(childnode);
+						talloc_free(childname);
+						ret = ENOMEM;
+						break;
+					}
+					ret = check_store_(childname,
+							   reachable);
 				}
-			}
-			else {
+			} else if (errno != ENOMEM) {
 				log("check_store: No child '%s' found!\n",
 				    childname);
 
@@ -1648,6 +1743,9 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					remove_child_entry(NULL, node, i);
 					i -= childlen + 1;
 				}
+			} else {
+				log("check_store: ENOMEM");
+				ret = ENOMEM;
 			}
 
 			talloc_free(childnode);
@@ -1658,14 +1756,18 @@ static void check_store_(const char *name, struct hashtable *reachable)
 		hashtable_destroy(children, 0 /* Don't free values (they are
 						 all (void *)1) */);
 		talloc_free(node);
-	}
-	else {
+	} else if (errno != ENOMEM) {
 		/* Impossible, because no database should ever be without the
 		   root, and otherwise, we've just checked in our caller
 		   (which made a recursive call to get here). */
 		   
 		log("check_store: No child '%s' found: impossible!", name);
+	} else {
+		log("check_store: ENOMEM");
+		ret = ENOMEM;
 	}
+
+	return ret;
 }
 
 
@@ -1678,6 +1780,11 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val,
 	struct hashtable *reachable = private;
 	char * name = talloc_strndup(NULL, key.dptr, key.dsize);
 
+	if (!name) {
+		log("clean_store: ENOMEM");
+		return 1;
+	}
+
 	if (!hashtable_search(reachable, name)) {
 		log("clean_store: '%s' is orphaned!", name);
 		if (recovery) {
@@ -1707,9 +1814,14 @@ static void check_store(void)
 	struct hashtable * reachable =
 		create_hashtable(16, hash_from_key_fn, keys_equal_fn);
  
+	if (!reachable) {
+		log("check_store: ENOMEM");
+		return;
+	}
+
 	log("Checking store ...");
-	check_store_(root, reachable);
-	clean_store(reachable);
+	if (!check_store_(root, reachable))
+		clean_store(reachable);
 	log("Checking store complete.");
 
 	hashtable_destroy(reachable, 0 /* Don't free values (they are all
@@ -1763,10 +1875,14 @@ static void init_sockets(int **psock, int **pro_sock)
 
 	/* Create sockets for them to listen to. */
 	*psock = sock = talloc(talloc_autofree_context(), int);
+	if (!sock)
+		barf_perror("No memory when creating sockets");
 	*sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (*sock < 0)
 		barf_perror("Could not create socket");
 	*pro_sock = ro_sock = talloc(talloc_autofree_context(), int);
+	if (!ro_sock)
+		barf_perror("No memory when creating sockets");
 	*ro_sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (*ro_sock < 0)
 		barf_perror("Could not create socket");
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index fefed5e..5322280 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -279,10 +279,15 @@ static struct domain *new_domain(void *context, unsigned int domid,
 	int rc;
 
 	domain = talloc(context, struct domain);
+	if (!domain)
+		return NULL;
+
 	domain->port = 0;
 	domain->shutdown = 0;
 	domain->domid = domid;
 	domain->path = talloc_domain_path(domain, domid);
+	if (!domain->path)
+		return NULL;
 
 	list_add(&domain->list, &domains);
 	talloc_set_destructor(domain, destroy_domain);
@@ -294,6 +299,9 @@ static struct domain *new_domain(void *context, unsigned int domid,
 	domain->port = rc;
 
 	domain->conn = new_connection(writechn, readchn);
+	if (!domain->conn)
+		return NULL;
+
 	domain->conn->domain = domain;
 	domain->conn->id = domid;
 
@@ -498,6 +506,8 @@ int do_get_domain_path(struct connection *conn, struct buffered_data *in)
 		return EINVAL;
 
 	path = talloc_domain_path(conn, atoi(domid_str));
+	if (!path)
+		return errno;
 
 	send_reply(conn, XS_GET_DOMAIN_PATH, path, strlen(path) + 1);
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index d314cd5..16f25fb 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -119,7 +119,18 @@ void add_change_node(struct connection *conn, struct node *node, bool recurse)
 	}
 
 	i = talloc(trans, struct changed_node);
+	if (!i) {
+		/* All we can do is let the transaction fail. */
+		generation++;
+		return;
+	}
 	i->node = talloc_strdup(i, node->name);
+	if (!i->node) {
+		/* All we can do is let the transaction fail. */
+		generation++;
+		talloc_free(i);
+		return;
+	}
 	i->recurse = recurse;
 	list_add_tail(&i->list, &trans->changes);
 }
@@ -163,11 +174,16 @@ int do_transaction_start(struct connection *conn, struct buffered_data *in)
 
 	/* Attach transaction to input for autofree until it's complete */
 	trans = talloc_zero(in, struct transaction);
+	if (!trans)
+		return ENOMEM;
+
 	INIT_LIST_HEAD(&trans->changes);
 	INIT_LIST_HEAD(&trans->changed_domains);
 	trans->generation = generation;
 	trans->tdb_name = talloc_asprintf(trans, "%s.%p",
 					  xs_daemon_tdb(), trans);
+	if (!trans->tdb_name)
+		return ENOMEM;
 	trans->tdb = tdb_copy(tdb_context(conn), trans->tdb_name);
 	if (!trans->tdb)
 		return errno;
@@ -246,6 +262,11 @@ void transaction_entry_inc(struct transaction *trans, unsigned int domid)
 		}
 
 	d = talloc(trans, struct changed_domain);
+	if (!d) {
+		/* Let the transaction fail. */
+		generation++;
+		return;
+	}
 	d->domid = domid;
 	d->nbentry = 1;
 	list_add_tail(&d->list, &trans->changed_domains);
@@ -262,6 +283,11 @@ void transaction_entry_dec(struct transaction *trans, unsigned int domid)
 		}
 
 	d = talloc(trans, struct changed_domain);
+	if (!d) {
+		/* Let the transaction fail. */
+		generation++;
+		return;
+	}
 	d->domid = domid;
 	d->nbentry = -1;
 	list_add_tail(&d->list, &trans->changed_domains);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 94251db..0dc5a40 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -111,6 +111,8 @@ static void add_event(struct connection *conn,
 
 	len = strlen(name) + 1 + strlen(watch->token) + 1;
 	data = talloc_array(ctx, char, len);
+	if (!data)
+		return;
 	strcpy(data, name);
 	strcpy(data + strlen(name) + 1, watch->token);
 	send_reply(conn, XS_WATCH_EVENT, data, len);
@@ -165,6 +167,8 @@ int do_watch(struct connection *conn, struct buffered_data *in)
 	} else {
 		relative = !strstarts(vec[0], "/");
 		vec[0] = canonicalize(conn, in, vec[0]);
+		if (!vec[0])
+			return ENOMEM;
 		if (!is_valid_nodename(vec[0]))
 			return EINVAL;
 	}
@@ -180,8 +184,14 @@ int do_watch(struct connection *conn, struct buffered_data *in)
 		return E2BIG;
 
 	watch = talloc(conn, struct watch);
+	if (!watch)
+		return ENOMEM;
 	watch->node = talloc_strdup(watch, vec[0]);
 	watch->token = talloc_strdup(watch, vec[1]);
+	if (!watch->node || !watch->token) {
+		talloc_free(watch);
+		return ENOMEM;
+	}
 	if (relative)
 		watch->relative_path = get_implicit_path(conn);
 	else
@@ -210,6 +220,8 @@ int do_unwatch(struct connection *conn, struct buffered_data *in)
 		return EINVAL;
 
 	node = canonicalize(conn, in, vec[0]);
+	if (!node)
+		return ENOMEM;
 	list_for_each_entry(watch, &conn->watches, list) {
 		if (streq(watch->node, node) && streq(watch->token, vec[1])) {
 			list_del(&watch->list);
-- 
2.10.2


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

Re: [PATCH v4 12/12] xenstore: handle memory allocation failures in xenstored

[Wei Liu]

On Mon, Dec 05, 2016 at 08:48:53AM +0100, Juergen Gross wrote:
> Check for failures when allocating new memory in xenstored.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I acked v3 of this patch already.

Assuming only the stale paragraph in commit message is deleted:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v4 12/12] xenstore: handle memory allocation failures in xenstored

[Juergen Gross]

On 05/12/16 11:03, Wei Liu wrote:
> On Mon, Dec 05, 2016 at 08:48:53AM +0100, Juergen Gross wrote:
>> Check for failures when allocating new memory in xenstored.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I acked v3 of this patch already.
> 
> Assuming only the stale paragraph in commit message is deleted:

Hmm, nearly. Did you miss:

V4: add alloc failure check in delete_node() and add_change_node()
    clean_store() only if no failure in check_store_()

> Acked-by: Wei Liu <wei.liu2@citrix.com>

Does this still stand then?


Juergen

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

Re: [PATCH v4 12/12] xenstore: handle memory allocation failures in xenstored

[Wei Liu]

On Mon, Dec 05, 2016 at 11:15:03AM +0100, Juergen Gross wrote:
> On 05/12/16 11:03, Wei Liu wrote:
> > On Mon, Dec 05, 2016 at 08:48:53AM +0100, Juergen Gross wrote:
> >> Check for failures when allocating new memory in xenstored.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > I acked v3 of this patch already.
> > 
> > Assuming only the stale paragraph in commit message is deleted:
> 
> Hmm, nearly. Did you miss:
> 
> V4: add alloc failure check in delete_node() and add_change_node()
>     clean_store() only if no failure in check_store_()

Urgh, -ENOCOFFEE and jetlag!

> 
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> Does this still stand then?
> 

I will have a look later. Sorry!

Wei.

> 
> Juergen

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

Re: [PATCH v4 12/12] xenstore: handle memory allocation failures in xenstored

[Wei Liu]

On Mon, Dec 05, 2016 at 10:16:39AM +0000, Wei Liu wrote:
> On Mon, Dec 05, 2016 at 11:15:03AM +0100, Juergen Gross wrote:
> > On 05/12/16 11:03, Wei Liu wrote:
> > > On Mon, Dec 05, 2016 at 08:48:53AM +0100, Juergen Gross wrote:
> > >> Check for failures when allocating new memory in xenstored.
> > >>
> > >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > > 
> > > I acked v3 of this patch already.
> > > 
> > > Assuming only the stale paragraph in commit message is deleted:
> > 
> > Hmm, nearly. Did you miss:
> > 
> > V4: add alloc failure check in delete_node() and add_change_node()
> >     clean_store() only if no failure in check_store_()
> 
> Urgh, -ENOCOFFEE and jetlag!
> 
> > 
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Does this still stand then?
> > 
> 
> I will have a look later. Sorry!

After a second look:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v4 00/12] xenstore: support reading directory with many children

[Wei Liu]

On Mon, Dec 05, 2016 at 08:48:41AM +0100, Juergen Gross wrote:
[...]
> Juergen Gross (12):
>   xenstore: modify add_change_node() parameter types
>   xenstore: call add_change_node() directly when writing node
>   xenstore: use common tdb record header in xenstore
>   xenstore: add per-node generation counter
>   xenstore: add support for reading directory with many children
>   xenstore: support XS_DIRECTORY_PART in libxenstore
>   xenstore: use array for xenstore wire command handling
>   xenstore: let command functions return error or success
>   xenstore: make functions static
>   xenstore: add helper functions for wire argument parsing
>   xenstore: add small default data buffer to internal struct
>   xenstore: handle memory allocation failures in xenstored
> 

Applied to staging.

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

Re: [PATCH v4 00/12] xenstore: support reading directory with many children

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1998 bytes --]

On 05/12/16 12:05, Wei Liu wrote:
> On Mon, Dec 05, 2016 at 08:48:41AM +0100, Juergen Gross wrote:
> [...]
>> Juergen Gross (12):
>>   xenstore: modify add_change_node() parameter types
>>   xenstore: call add_change_node() directly when writing node
>>   xenstore: use common tdb record header in xenstore
>>   xenstore: add per-node generation counter
>>   xenstore: add support for reading directory with many children
>>   xenstore: support XS_DIRECTORY_PART in libxenstore
>>   xenstore: use array for xenstore wire command handling
>>   xenstore: let command functions return error or success
>>   xenstore: make functions static
>>   xenstore: add helper functions for wire argument parsing
>>   xenstore: add small default data buffer to internal struct
>>   xenstore: handle memory allocation failures in xenstored
>>
> Applied to staging.

XenServer's Coverity has run, and has a few things to say.  Its not
obvious (i.e. I can't trivially identify) if these are preexisting bugs
which your code has brought to light, or introduced by your series.

Both do_rm() and do_mkdir() suffer from the same problem.

onearg(in) may return NULL, which results in get_node_canonicalized()
setting name to NULL and returning NULL.  name is then dereferenced in
the error path by get_parent()/create_node() respectively.

There are two complains of leaking a talloc_array() allocation.  First
in do_set_perms() via the xs_strings_to_perms() failure path and second
in send_directory_part() via the success path.  The use of
talloc_array() in add_event() would suggest that you are expected to
call talloc_free() on the result, unless I am missing something subtle.

There is another resource leak complaint of bdata in send_reply(),
although this case clearly hasn't observed the list_add_tail(), so I
don't think this is a real issue.

Finally, xs_directory_part(), on line 619 uses a strncpy() for all bytes
of gen, but doesn't necesserily NULL terminate it, and is later used by
strcmp().

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2769 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v4 00/12] xenstore: support reading directory with many children

[Juergen Gross]

On 05/12/16 19:19, Andrew Cooper wrote:
> On 05/12/16 12:05, Wei Liu wrote:
>> On Mon, Dec 05, 2016 at 08:48:41AM +0100, Juergen Gross wrote:
>> [...]
>>> Juergen Gross (12):
>>>   xenstore: modify add_change_node() parameter types
>>>   xenstore: call add_change_node() directly when writing node
>>>   xenstore: use common tdb record header in xenstore
>>>   xenstore: add per-node generation counter
>>>   xenstore: add support for reading directory with many children
>>>   xenstore: support XS_DIRECTORY_PART in libxenstore
>>>   xenstore: use array for xenstore wire command handling
>>>   xenstore: let command functions return error or success
>>>   xenstore: make functions static
>>>   xenstore: add helper functions for wire argument parsing
>>>   xenstore: add small default data buffer to internal struct
>>>   xenstore: handle memory allocation failures in xenstored
>>>
>> Applied to staging.
> 
> XenServer's Coverity has run, and has a few things to say.  Its not
> obvious (i.e. I can't trivially identify) if these are preexisting bugs
> which your code has brought to light, or introduced by your series.
> 
> Both do_rm() and do_mkdir() suffer from the same problem.
> 
> onearg(in) may return NULL, which results in get_node_canonicalized()
> setting name to NULL and returning NULL.  name is then dereferenced in
> the error path by get_parent()/create_node() respectively.

No. errno will be EINVAL and this will prohibit to enter the said paths
guarded by errno == ENOENT.

> There are two complains of leaking a talloc_array() allocation.  First
> in do_set_perms() via the xs_strings_to_perms() failure path and second

No. The allocation context is "node" which is allocated using "in" as
allocation context. This is freed when the command has been processed.

> in send_directory_part() via the success path.  The use of

No again. Allocation context is "in" to be freed after processing the
command.

> talloc_array() in add_event() would suggest that you are expected to
> call talloc_free() on the result, unless I am missing something subtle.

You do. The free in add_event() is done as the allocation context might
be NULL (e.g. when called from domain_cleanup()). So here the free is
really needed in order to avoid a leak.

> There is another resource leak complaint of bdata in send_reply(),
> although this case clearly hasn't observed the list_add_tail(), so I
> don't think this is a real issue.

Right.

> Finally, xs_directory_part(), on line 619 uses a strncpy() for all bytes
> of gen, but doesn't necesserily NULL terminate it, and is later used by
> strcmp().

This is a real issue. I'll send a patch.


Juergen

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

Re: [PATCH v4 00/12] xenstore: support reading directory with many children

[Andrew Cooper]

On 06/12/16 06:30, Juergen Gross wrote:
> On 05/12/16 19:19, Andrew Cooper wrote:
>> On 05/12/16 12:05, Wei Liu wrote:
>>> On Mon, Dec 05, 2016 at 08:48:41AM +0100, Juergen Gross wrote:
>>> [...]
>>>> Juergen Gross (12):
>>>>   xenstore: modify add_change_node() parameter types
>>>>   xenstore: call add_change_node() directly when writing node
>>>>   xenstore: use common tdb record header in xenstore
>>>>   xenstore: add per-node generation counter
>>>>   xenstore: add support for reading directory with many children
>>>>   xenstore: support XS_DIRECTORY_PART in libxenstore
>>>>   xenstore: use array for xenstore wire command handling
>>>>   xenstore: let command functions return error or success
>>>>   xenstore: make functions static
>>>>   xenstore: add helper functions for wire argument parsing
>>>>   xenstore: add small default data buffer to internal struct
>>>>   xenstore: handle memory allocation failures in xenstored
>>>>
>>> Applied to staging.
>> XenServer's Coverity has run, and has a few things to say.  Its not
>> obvious (i.e. I can't trivially identify) if these are preexisting bugs
>> which your code has brought to light, or introduced by your series.
>>
>> Both do_rm() and do_mkdir() suffer from the same problem.
>>
>> onearg(in) may return NULL, which results in get_node_canonicalized()
>> setting name to NULL and returning NULL.  name is then dereferenced in
>> the error path by get_parent()/create_node() respectively.
> No. errno will be EINVAL and this will prohibit to enter the said paths
> guarded by errno == ENOENT.

It looks like Coverity is finding reason to presume that errno == ENOENT
on the bad path.

I think this is because it cant conclude whether setting name to NULL is
strictly equivalent to setting errno to ENOENT or not, and errs on the
side of assuming not.

~Andrew

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