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

[PATCH v3 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 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  |  24 +-
 tools/xenstore/xenstore_client.c       | 117 +++++
 tools/xenstore/xenstored_core.c        | 799 ++++++++++++++++++---------------
 tools/xenstore/xenstored_core.h        |  23 +-
 tools/xenstore/xenstored_domain.c      | 218 ++++-----
 tools/xenstore/xenstored_domain.h      |  14 +-
 tools/xenstore/xenstored_transaction.c | 100 +++--
 tools/xenstore/xenstored_transaction.h |   8 +-
 tools/xenstore/xenstored_watch.c       |  89 ++--
 tools/xenstore/xenstored_watch.h       |   6 +-
 tools/xenstore/xs.c                    |  80 +++-
 tools/xenstore/xs_lib.c                | 112 -----
 tools/xenstore/xs_tdb_dump.c           |  11 +-
 xen/include/public/io/xs_wire.h        |   3 +
 14 files changed, 872 insertions(+), 732 deletions(-)

-- 
2.6.6


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

[PATCH v3 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>
---
 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.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 08:59:59AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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().

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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote:
> 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>


There  seems to be a subtle change in behaviour -- previously in
create_node, there is no add_chnage_node called. Now it has.

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

Another subtle change of behaviour -- there is another goto error after
this, which means the change is not made as far as the caller is
concerned if that path is taken.

Not saying that all these changes are wrong, but they are worth pointing
out and probably we should put the reasoning into commit message.

>  	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

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

[Juergen Gross]

On 12/11/16 16:10, Wei Liu wrote:
> On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote:
>> 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>
> 
> 
> There  seems to be a subtle change in behaviour -- previously in
> create_node, there is no add_chnage_node called. Now it has.

No change. add_change_node() has been called after calling create_node()

> 
>> ---
>>  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);
>> +
> 
> Another subtle change of behaviour -- there is another goto error after
> this, which means the change is not made as far as the caller is
> concerned if that path is taken.

That's a case which should not be of any concern: it means that
the xenstore data base is corrupt. A stale watch event is the
least problem then.

> Not saying that all these changes are wrong, but they are worth pointing
> out and probably we should put the reasoning into commit message.

I'll add some words for this case.


Juergen

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

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

[Wei Liu]

On Sun, Nov 13, 2016 at 11:50:27AM +0100, Juergen Gross wrote:
> On 12/11/16 16:10, Wei Liu wrote:
> > On Fri, Nov 11, 2016 at 09:00:00AM +0100, Juergen Gross wrote:
> >> 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>
> > 
> > 
> > There  seems to be a subtle change in behaviour -- previously in
> > create_node, there is no add_chnage_node called. Now it has.
> 
> No change. add_change_node() has been called after calling create_node()
> 

I don't think that's true for all cases. For example, setup_structure
calls create_node but doesn't call add_change_node after that.

But I have convinced myself that it would be fine to call
add_change_node in write_node because that seems rather logical --
writing a node is certainly changing it.

Wei.

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

[PATCH v3 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>
---
 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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:01AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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>
---
 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.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:02AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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>
---
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()

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c | 67 +++++++++++++++++++++++++++++++++++++++++
 xen/include/public/io/xs_wire.h |  3 ++
 2 files changed, 70 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..9a6f8eb 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -50,6 +50,9 @@ enum xsd_sockmsg_type
     XS_SET_TARGET,
     XS_RESTRICT,
     XS_RESET_WATCHES,
+    XS_DIRECTORY_PART,
+
+    XS_NEXT_ENTRY,      /* First unused type. */
 
     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 v3 05/12] xenstore: add support for reading directory with many children

[Jan Beulich]

>>> On 11.11.16 at 09:00, <JGross@suse.com> wrote:
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -50,6 +50,9 @@ enum xsd_sockmsg_type
>      XS_SET_TARGET,
>      XS_RESTRICT,
>      XS_RESET_WATCHES,
> +    XS_DIRECTORY_PART,
> +
> +    XS_NEXT_ENTRY,      /* First unused type. */

What is this needed for?

Jan


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

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

[Juergen Gross]

On 11/11/16 11:09, Jan Beulich wrote:
>>>> On 11.11.16 at 09:00, <JGross@suse.com> wrote:
>> --- a/xen/include/public/io/xs_wire.h
>> +++ b/xen/include/public/io/xs_wire.h
>> @@ -50,6 +50,9 @@ enum xsd_sockmsg_type
>>      XS_SET_TARGET,
>>      XS_RESTRICT,
>>      XS_RESET_WATCHES,
>> +    XS_DIRECTORY_PART,
>> +
>> +    XS_NEXT_ENTRY,      /* First unused type. */
> 
> What is this needed for?

Patch 7. I didn't want to modify the same enum twice in the same series.
In case you'd prefer to add XS_NEXT_ENTRY in patch 7 I'd be happy to
move it.


Juergen


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

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

[Jan Beulich]

>>> On 11.11.16 at 11:43, <JGross@suse.com> wrote:
> On 11/11/16 11:09, Jan Beulich wrote:
>>>>> On 11.11.16 at 09:00, <JGross@suse.com> wrote:
>>> --- a/xen/include/public/io/xs_wire.h
>>> +++ b/xen/include/public/io/xs_wire.h
>>> @@ -50,6 +50,9 @@ enum xsd_sockmsg_type
>>>      XS_SET_TARGET,
>>>      XS_RESTRICT,
>>>      XS_RESET_WATCHES,
>>> +    XS_DIRECTORY_PART,
>>> +
>>> +    XS_NEXT_ENTRY,      /* First unused type. */
>> 
>> What is this needed for?
> 
> Patch 7. I didn't want to modify the same enum twice in the same series.
> In case you'd prefer to add XS_NEXT_ENTRY in patch 7 I'd be happy to
> move it.

Yes please. Additions should be made where they are needed;
please don't forget that series' may be committed in pieces. Also
judging about the chosen name (which I consider somewhat odd -
I'd have expected something like XS_TYPE_COUNT) is easier when
one sees the intended use.

Jan


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

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

[Juergen Gross]

On 11/11/16 12:12, Jan Beulich wrote:
>>>> On 11.11.16 at 11:43, <JGross@suse.com> wrote:
>> On 11/11/16 11:09, Jan Beulich wrote:
>>>>>> On 11.11.16 at 09:00, <JGross@suse.com> wrote:
>>>> --- a/xen/include/public/io/xs_wire.h
>>>> +++ b/xen/include/public/io/xs_wire.h
>>>> @@ -50,6 +50,9 @@ enum xsd_sockmsg_type
>>>>      XS_SET_TARGET,
>>>>      XS_RESTRICT,
>>>>      XS_RESET_WATCHES,
>>>> +    XS_DIRECTORY_PART,
>>>> +
>>>> +    XS_NEXT_ENTRY,      /* First unused type. */
>>>
>>> What is this needed for?
>>
>> Patch 7. I didn't want to modify the same enum twice in the same series.
>> In case you'd prefer to add XS_NEXT_ENTRY in patch 7 I'd be happy to
>> move it.
> 
> Yes please. Additions should be made where they are needed;
> please don't forget that series' may be committed in pieces. Also
> judging about the chosen name (which I consider somewhat odd -
> I'd have expected something like XS_TYPE_COUNT) is easier when
> one sees the intended use.

Okay to both (moving and renaming to your suggested name).


Juergen


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:03AM +0100, Juergen Gross wrote:
> 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>

(with Jan's comment addressed)

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

[PATCH v3 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>
---
 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 v3 06/12] xenstore: support XS_DIRECTORY_PART in libxenstore

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:04AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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>
---
 tools/xenstore/xenstored_core.c | 158 +++++++++++-----------------------------
 1 file changed, 44 insertions(+), 114 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index e4e09fa..f46ba6f 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_NEXT_ENTRY] = {
+	[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_NEXT_ENTRY && 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_NEXT_ENTRY && 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;
-- 
2.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:05AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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().

Signed-off-by: Juergen Gross <jgross@suse.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 f46ba6f..f5ec585 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_NEXT_ENTRY] = {
 	[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_NEXT_ENTRY && 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.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:06AM +0100, Juergen Gross wrote:
> 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().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

As far as I can tell, this is merely refactoring existing code and has
no functional change -- we should probably say so in the commit message.

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 v3 08/12] xenstore: let command functions return error or success

[Juergen Gross]

On 12/11/16 16:11, Wei Liu wrote:
> On Fri, Nov 11, 2016 at 09:00:06AM +0100, Juergen Gross wrote:
>> 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().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> As far as I can tell, this is merely refactoring existing code and has
> no functional change -- we should probably say so in the commit message.

Okay.


Juergen

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


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

[PATCH v3 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>
---
 tools/xenstore/include/xenstore_lib.h |  15 -----
 tools/xenstore/xenstore_client.c      | 117 ++++++++++++++++++++++++++++++++++
 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_lib.c               | 112 --------------------------------
 7 files changed, 159 insertions(+), 183 deletions(-)

diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h
index 0ffbae9..866261d 100644
--- a/tools/xenstore/include/xenstore_lib.h
+++ b/tools/xenstore/include/xenstore_lib.h
@@ -76,19 +76,4 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
 /* Given a string and a length, count how many strings (nul terms). */
 unsigned int xs_count_strings(const char *strings, unsigned int len);
 
-/* Sanitising (quoting) possibly-binary strings. */
-struct expanding_buffer {
-	char *buf;
-	int avail;
-};
-
-/* Ensure that given expanding buffer has at least min_avail characters. */
-char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
-
-/* sanitise_value() may return NULL if malloc fails. */
-char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
-
-/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
-
 #endif /* XENSTORE_LIB_H */
diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 3d14d37..dfa75a7 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -10,6 +10,7 @@
 
 #include <err.h>
 #include <errno.h>
+#include <assert.h>
 #include <fcntl.h>
 #include <getopt.h>
 #include <stdarg.h>
@@ -38,12 +39,128 @@ enum mode {
     MODE_watch,
 };
 
+/* Sanitising (quoting) possibly-binary strings. */
+struct expanding_buffer {
+	char *buf;
+	int avail;
+};
+
 static char *output_buf = NULL;
 static int output_pos = 0;
 static struct expanding_buffer ebuf;
 
 static int output_size = 0;
 
+static 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;
+}
+
+static 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
+}
+
+static 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;
+}
 static void
 output(const char *fmt, ...) {
     va_list ap;
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f5ec585..3faab6e 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_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.6.6


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

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

[Andrew Cooper]

On 11/11/16 08:00, 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>
> ---
>  tools/xenstore/include/xenstore_lib.h |  15 -----
>  tools/xenstore/xenstore_client.c      | 117 ++++++++++++++++++++++++++++++++++
>  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_lib.c               | 112 --------------------------------
>  7 files changed, 159 insertions(+), 183 deletions(-)
>
> diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h
> index 0ffbae9..866261d 100644
> --- a/tools/xenstore/include/xenstore_lib.h
> +++ b/tools/xenstore/include/xenstore_lib.h
> @@ -76,19 +76,4 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
>  /* Given a string and a length, count how many strings (nul terms). */
>  unsigned int xs_count_strings(const char *strings, unsigned int len);
>  
> -/* Sanitising (quoting) possibly-binary strings. */
> -struct expanding_buffer {
> -	char *buf;
> -	int avail;
> -};
> -
> -/* Ensure that given expanding buffer has at least min_avail characters. */
> -char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
> -
> -/* sanitise_value() may return NULL if malloc fails. */
> -char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
> -
> -/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
> -void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
> -
>  #endif /* XENSTORE_LIB_H */

This is a public header file for a shared object.  You can't remove
functionality like this.

~Andrew

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

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

[Juergen Gross]

On 11/11/16 14:02, Andrew Cooper wrote:
> On 11/11/16 08:00, 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>
>> ---
>>  tools/xenstore/include/xenstore_lib.h |  15 -----
>>  tools/xenstore/xenstore_client.c      | 117 ++++++++++++++++++++++++++++++++++
>>  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_lib.c               | 112 --------------------------------
>>  7 files changed, 159 insertions(+), 183 deletions(-)
>>
>> diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h
>> index 0ffbae9..866261d 100644
>> --- a/tools/xenstore/include/xenstore_lib.h
>> +++ b/tools/xenstore/include/xenstore_lib.h
>> @@ -76,19 +76,4 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
>>  /* Given a string and a length, count how many strings (nul terms). */
>>  unsigned int xs_count_strings(const char *strings, unsigned int len);
>>  
>> -/* Sanitising (quoting) possibly-binary strings. */
>> -struct expanding_buffer {
>> -	char *buf;
>> -	int avail;
>> -};
>> -
>> -/* Ensure that given expanding buffer has at least min_avail characters. */
>> -char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
>> -
>> -/* sanitise_value() may return NULL if malloc fails. */
>> -char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
>> -
>> -/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
>> -void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
>> -
>>  #endif /* XENSTORE_LIB_H */
> 
> This is a public header file for a shared object.  You can't remove
> functionality like this.

Oops, my bad. I stopped reading the Makefile at the xenstored runes
telling me xs_lib.o to be linked into xenstored. I assumed the same to
be true for xenstore_client. So I think I'll modify the patch to avoid
including the not needed parts of xs_lib.c in xenstored.


Juergen

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

[PATCH v3 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>
---
 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 3faab6e..c7a7c45 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.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:08AM +0100, Juergen Gross wrote:
> 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>

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

[PATCH v3 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>
---
 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 c7a7c45..f89a636 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.6.6


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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 09:00:09AM +0100, Juergen Gross wrote:
> 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>

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

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

[Juergen Gross]

Check for failures when allocating new memory in xenstored.

Unfortunately there are several conditions which might render those
checks void: It might be impossible to send an "ENOMEM" response as
this requires allocating some memory.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c        | 185 ++++++++++++++++++++++++++-------
 tools/xenstore/xenstored_domain.c      |  10 ++
 tools/xenstore/xenstored_transaction.c |  20 ++++
 tools/xenstore/xenstored_watch.c       |  12 +++
 4 files changed, 187 insertions(+), 40 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f89a636..d880afa 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)
@@ -1086,9 +1137,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 +1171,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 +1248,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 +1383,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 +1391,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 +1536,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 +1576,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 +1656,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);
 }
 
 
@@ -1615,14 +1690,23 @@ 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;
+		}
 
 		while (i < node->childlen) {
+			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");
+				break;
+			}
+			childnode = read_node(NULL, childname, childname);
 			
 			if (childnode) {
 				if (hashtable_search(children, childname)) {
@@ -1636,11 +1720,16 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					}
 				}
 				else {
-					remember_string(children, childname);
+					if (!remember_string(children,
+							     childname)) {
+						log("check_store: ENOMEM");
+						talloc_free(childnode);
+						talloc_free(childname);
+						break;
+					}
 					check_store_(childname, reachable);
 				}
-			}
-			else {
+			} else if (errno != ENOMEM) {
 				log("check_store: No child '%s' found!\n",
 				    childname);
 
@@ -1648,7 +1737,8 @@ static void check_store_(const char *name, struct hashtable *reachable)
 					remove_child_entry(NULL, node, i);
 					i -= childlen + 1;
 				}
-			}
+			} else
+				log("check_store: ENOMEM");
 
 			talloc_free(childnode);
 			talloc_free(childname);
@@ -1658,14 +1748,15 @@ 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");
+
 }
 
 
@@ -1678,6 +1769,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,6 +1803,11 @@ 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);
@@ -1763,10 +1864,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..ff05f95 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -119,6 +119,11 @@ 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);
 	i->recurse = recurse;
 	list_add_tail(&i->list, &trans->changes);
@@ -163,11 +168,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 +256,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 +277,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.6.6


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

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

[Juergen Gross]

On 11/11/16 09:00, Juergen Gross wrote:
> Check for failures when allocating new memory in xenstored.
> 
> Unfortunately there are several conditions which might render those
> checks void: It might be impossible to send an "ENOMEM" response as
> this requires allocating some memory.

Uuh, sorry. This paragraph can be deleted, as patch 11 now takes
care of the situation.

In case another round is needed I'll modify the commit message.
Otherwise I can either resend just this patch if necessary.


Juergen

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

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

[Wei Liu]

On Fri, Nov 11, 2016 at 12:07:16PM +0100, Juergen Gross wrote:
> On 11/11/16 09:00, Juergen Gross wrote:
> > Check for failures when allocating new memory in xenstored.
> > 
> > Unfortunately there are several conditions which might render those
> > checks void: It might be impossible to send an "ENOMEM" response as
> > this requires allocating some memory.
> 
> Uuh, sorry. This paragraph can be deleted, as patch 11 now takes
> care of the situation.
> 
> In case another round is needed I'll modify the commit message.
> Otherwise I can either resend just this patch if necessary.
> 

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

> 
> Juergen

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