[PATCH v2 0/7] xenstore: support reading directory with many children

[PATCH v2 0/7] 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.

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 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 (7):
  xenstore: fix add_change_node()
  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

 tools/xenstore/include/xenstore_lib.h  |   9 +++
 tools/xenstore/xenstored_core.c        | 120 ++++++++++++++++++++++++++-------
 tools/xenstore/xenstored_core.h        |   3 +
 tools/xenstore/xenstored_transaction.c |  27 +++++---
 tools/xenstore/xenstored_transaction.h |   4 +-
 tools/xenstore/xs.c                    |  80 +++++++++++++++++++---
 tools/xenstore/xs_tdb_dump.c           |  11 +--
 xen/include/public/io/xs_wire.h        |   1 +
 8 files changed, 205 insertions(+), 50 deletions(-)

-- 
2.6.6


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

[PATCH v2 1/7] xenstore: fix add_change_node()

[Juergen Gross]

add_change_node() in xenstored is used to add a modified node to the
list of changed nodes of one transaction. It is being called with the
recurse parameter set to true when removing a node in order to get
watches for children of the node fired at transaction end, too.

If, however, the node to be deleted had been modified in the same
transaction the recurse parameter of add_change_node() is lost as
an entry already in the list of the changed nodes won't be entered
again.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
Candidate for backport
---
 tools/xenstore/xenstored_transaction.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 34720fa..3c0b8f7 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -103,8 +103,11 @@ void add_change_node(struct transaction *trans, const char *node, bool recurse)
 	}
 
 	list_for_each_entry(i, &trans->changes, list)
-		if (streq(i->node, node))
+		if (streq(i->node, node)) {
+			if (recurse)
+				i->recurse = recurse;
 			return;
+		}
 
 	i = talloc(trans, struct changed_node);
 	i->node = talloc_strdup(i, node);
-- 
2.6.6


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

[PATCH v2 2/7] 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>
---
 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 3c0b8f7..39996f1 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -92,25 +92,28 @@ 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;
 		}
 
 	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.6.6


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

[PATCH v2 3/7] 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().

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.6.6


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

[PATCH v2 4/7] 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>
---
 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.6.6


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

[PATCH v2 5/7] 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>
---
 tools/xenstore/include/xenstore_lib.h  |  1 +
 tools/xenstore/xenstored_core.c        |  2 ++
 tools/xenstore/xenstored_core.h        |  3 +++
 tools/xenstore/xenstored_transaction.c | 13 +++++++++----
 4 files changed, 15 insertions(+), 4 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 39996f1..39e5a49 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)
@@ -234,7 +239,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.6.6


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

[PATCH v2 6/7] 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>
---
 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..ecee4e2 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>
@@ -812,6 +813,68 @@ 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) {
+		len = strlen(gen) + 2;
+		gen[len - 1] = 0;
+		send_reply(conn, XS_DIRECTORY_PART, gen, len);
+		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;
+	}
+
+	strcpy(data, gen);
+	memcpy(data + genlen, node->children + off, len);
+	if (off + len == node->childlen) {
+		len++;
+		data[genlen + len] = 0;
+	}
+
+	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.6.6


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

[PATCH v2 7/7] 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>
---
 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.6.6


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

Re: [PATCH v2 1/7] xenstore: fix add_change_node()

[Wei Liu]

On Thu, Oct 27, 2016 at 11:55:52AM +0200, Juergen Gross wrote:
> add_change_node() in xenstored is used to add a modified node to the
> list of changed nodes of one transaction. It is being called with the
> recurse parameter set to true when removing a node in order to get
> watches for children of the node fired at transaction end, too.
> 
> If, however, the node to be deleted had been modified in the same
> transaction the recurse parameter of add_change_node() is lost as
> an entry already in the list of the changed nodes won't be entered
> again.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

I think we should apply this to 4.8, too. I will wait a bit for
different opinions.

(I've only looked at this patch in this series because I caught the
"fix" in subject line)

> ---
> Candidate for backport
> ---
>  tools/xenstore/xenstored_transaction.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
> index 34720fa..3c0b8f7 100644
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -103,8 +103,11 @@ void add_change_node(struct transaction *trans, const char *node, bool recurse)
>  	}
>  
>  	list_for_each_entry(i, &trans->changes, list)
> -		if (streq(i->node, node))
> +		if (streq(i->node, node)) {
> +			if (recurse)
> +				i->recurse = recurse;
>  			return;
> +		}

I would like to add {} around list_for_each_entry. No need to resend.

Wei.

>  
>  	i = talloc(trans, struct changed_node);
>  	i->node = talloc_strdup(i, node);
> -- 
> 2.6.6
> 

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

Re: [PATCH v2 1/7] xenstore: fix add_change_node()

[Juergen Gross]

On 27/10/16 12:15, Wei Liu wrote:
> On Thu, Oct 27, 2016 at 11:55:52AM +0200, Juergen Gross wrote:
>> add_change_node() in xenstored is used to add a modified node to the
>> list of changed nodes of one transaction. It is being called with the
>> recurse parameter set to true when removing a node in order to get
>> watches for children of the node fired at transaction end, too.
>>
>> If, however, the node to be deleted had been modified in the same
>> transaction the recurse parameter of add_change_node() is lost as
>> an entry already in the list of the changed nodes won't be entered
>> again.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> I think we should apply this to 4.8, too. I will wait a bit for
> different opinions.
> 
> (I've only looked at this patch in this series because I caught the
> "fix" in subject line)

That's how it was meant to be. I just wanted to get some feedback
about the general idea before continuing the work on xenstored
(handling of memory allocation failures, more effective transaction
handling), as some of this work will be based on the per-node
generation counter I've introduced with this series.


Juergen


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

[PATCH v2 6/7] xenstore: add support for reading directory with many children

[Jan Beulich]

>>> On 27.10.16 at 11:55, <JGross@suse.com> wrote:
> +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);

Not being a user land person, I consider this too weak: Imo using
strtoul() and properly handling conversion errors would be better
here.

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

Why not use the shorter hex representation here?

> +	/* Offset behind list: just return a list with an empty string. */
> +	if (off >= node->childlen) {

Is it perhaps worth separating the == and > cases? The former is
just EOL, while the latter is really an error.

> +		len = strlen(gen) + 2;
> +		gen[len - 1] = 0;
> +		send_reply(conn, XS_DIRECTORY_PART, gen, len);

Any reason not to utilize genlen here, instead of the extra strlen()?

> +		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;
> +	}
> +
> +	strcpy(data, gen);

memcpy(data, gen, genlen); ?

> +	memcpy(data + genlen, node->children + off, len);
> +	if (off + len == node->childlen) {
> +		len++;
> +		data[genlen + len] = 0;
> +	}
> +
> +	send_reply(conn, XS_DIRECTORY_PART, data, genlen + len);
> +}

Since you don't return the offset, am I understanding right that the
remote side is expected to accumulate that value?

Jan

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

Re: [PATCH v2 6/7] xenstore: add support for reading directory with many children

[Juergen Gross]

On 27/10/16 15:56, Jan Beulich wrote:
>>>> On 27.10.16 at 11:55, <JGross@suse.com> wrote:
>> +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);
> 
> Not being a user land person, I consider this too weak: Imo using
> strtoul() and properly handling conversion errors would be better
> here.

Common practice in xenstored. In case the user is doing silly things
the worst case here is he will receive silly data (either an empty
children list or a list starting in the middle of a child's name).

>> +	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;
> 
> Why not use the shorter hex representation here?

Numbers are normally transferred in decimal representation (domid,
transaction id).

>> +	/* Offset behind list: just return a list with an empty string. */
>> +	if (off >= node->childlen) {
> 
> Is it perhaps worth separating the == and > cases? The former is
> just EOL, while the latter is really an error.

Hmm, yes. I'll modify it.

>> +		len = strlen(gen) + 2;
>> +		gen[len - 1] = 0;
>> +		send_reply(conn, XS_DIRECTORY_PART, gen, len);
> 
> Any reason not to utilize genlen here, instead of the extra strlen()?

No, you are right.

>> +		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;
>> +	}
>> +
>> +	strcpy(data, gen);
> 
> memcpy(data, gen, genlen); ?

I don't mind.

> 
>> +	memcpy(data + genlen, node->children + off, len);
>> +	if (off + len == node->childlen) {
>> +		len++;
>> +		data[genlen + len] = 0;
>> +	}
>> +
>> +	send_reply(conn, XS_DIRECTORY_PART, data, genlen + len);
>> +}
> 
> Since you don't return the offset, am I understanding right that the
> remote side is expected to accumulate that value?

Yes.


Thanks for the review,

Juergen

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

Re: [PATCH v2 6/7] xenstore: add support for reading directory with many children

[Juergen Gross]

On 27/10/16 17:00, Juergen Gross wrote:
> On 27/10/16 15:56, Jan Beulich wrote:
>>>>> On 27.10.16 at 11:55, <JGross@suse.com> wrote:
>>> +	/* Offset behind list: just return a list with an empty string. */
>>> +	if (off >= node->childlen) {
>>
>> Is it perhaps worth separating the == and > cases? The former is
>> just EOL, while the latter is really an error.
> 
> Hmm, yes. I'll modify it.

On a second thought: this might be the result of a concurrent
child removal. So the caller would have no chance to avoid this
situation, but he would see a different generation count in the
response.

I think I leave it as it is.


Juergen


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

Re: [PATCH v2 1/7] xenstore: fix add_change_node()

[Wei Liu]

On Thu, Oct 27, 2016 at 11:15:24AM +0100, Wei Liu wrote:
> On Thu, Oct 27, 2016 at 11:55:52AM +0200, Juergen Gross wrote:
> > add_change_node() in xenstored is used to add a modified node to the
> > list of changed nodes of one transaction. It is being called with the
> > recurse parameter set to true when removing a node in order to get
> > watches for children of the node fired at transaction end, too.
> > 
> > If, however, the node to be deleted had been modified in the same
> > transaction the recurse parameter of add_change_node() is lost as
> > an entry already in the list of the changed nodes won't be entered
> > again.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> I think we should apply this to 4.8, too. I will wait a bit for
> different opinions.
> 
> (I've only looked at this patch in this series because I caught the
> "fix" in subject line)
> 
> > ---
> > Candidate for backport
> > ---
> >  tools/xenstore/xenstored_transaction.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
> > index 34720fa..3c0b8f7 100644
> > --- a/tools/xenstore/xenstored_transaction.c
> > +++ b/tools/xenstore/xenstored_transaction.c
> > @@ -103,8 +103,11 @@ void add_change_node(struct transaction *trans, const char *node, bool recurse)
> >  	}
> >  
> >  	list_for_each_entry(i, &trans->changes, list)
> > -		if (streq(i->node, node))
> > +		if (streq(i->node, node)) {
> > +			if (recurse)
> > +				i->recurse = recurse;
> >  			return;
> > +		}
> 
> I would like to add {} around list_for_each_entry. No need to resend.
> 

Added pair of braces and applied.

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