* [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes
@ 2022-12-13 16:00 Juergen Gross
2022-12-13 16:00 ` [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno Juergen Gross
` (19 more replies)
0 siblings, 20 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD,
Andrew Cooper, George Dunlap, Jan Beulich, Stefano Stabellini
This is a first run of post-XSA patches which piled up during the
development phase of all the recent Xenstore related XSA patches.
At least the first 5 patches are completely independent from each
other. After those the dependencies are starting to be more complex.
This is a mixture of small fixes, enhancements and cleanups.
Changes in V2:
- patches 1+2 of V1 have been applied already
- addressed comments
- new patch 19
Juergen Gross (19):
tools/xenstore: let talloc_free() preserve errno
tools/xenstore: let tdb_logger() preserve errno
tools/xenstore: preserve errno across corrupt()
tools/xenstore: remove all watches when a domain has stopped
tools/xenstore: enhance hashtable implementation
tools/xenstore: add hashlist for finding struct domain by domid
tools/xenstore: introduce dummy nodes for special watch paths
tools/xenstore: replace watch->relative_path with a prefix length
tools/xenstore: move changed domain handling
tools/xenstore: change per-domain node accounting interface
tools/xenstore: don't allow creating too many nodes in a transaction
tools/xenstore: replace literal domid 0 with dom0_domid
tools/xenstore: make domain_is_unprivileged() an inline function
tools/xenstore: let chk_domain_generation() return a bool
tools/xenstore: switch hashtable to use the talloc framework
tools/xenstore: make log macro globally available
tools/xenstore: introduce trace classes
tools/xenstore: let check_store() check the accounting data
tools/xenstore: make output of "xenstore-control help" more pretty
docs/misc/xenstore.txt | 10 +-
tools/xenstore/hashtable.c | 134 +++---
tools/xenstore/hashtable.h | 38 +-
tools/xenstore/talloc.c | 21 +-
tools/xenstore/xenstored_control.c | 36 +-
tools/xenstore/xenstored_core.c | 259 +++++++----
tools/xenstore/xenstored_core.h | 31 ++
tools/xenstore/xenstored_domain.c | 609 +++++++++++++------------
tools/xenstore/xenstored_domain.h | 21 +-
tools/xenstore/xenstored_transaction.c | 76 +--
tools/xenstore/xenstored_transaction.h | 7 +-
tools/xenstore/xenstored_watch.c | 44 +-
12 files changed, 707 insertions(+), 579 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-14 9:54 ` Jan Beulich
2022-12-13 16:00 ` [PATCH v2 02/19] tools/xenstore: let tdb_logger() " Juergen Gross
` (18 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Today talloc_free() is not guaranteed to preserve errno, especially in
case a custom destructor is being used.
Change that by renaming talloc_free() to _talloc_free() in talloc.c and
adding a wrapper to talloc.c.
This allows to remove some errno saving outside of talloc.c.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- drop wrapper (Julien Grall)
---
tools/xenstore/talloc.c | 21 +++++++++++++--------
tools/xenstore/xenstored_core.c | 2 --
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/tools/xenstore/talloc.c b/tools/xenstore/talloc.c
index d7edcf3a93..23c3a23b19 100644
--- a/tools/xenstore/talloc.c
+++ b/tools/xenstore/talloc.c
@@ -541,38 +541,39 @@ static void talloc_free_children(void *ptr)
*/
int talloc_free(void *ptr)
{
+ int saved_errno = errno;
struct talloc_chunk *tc;
if (ptr == NULL) {
- return -1;
+ goto err;
}
tc = talloc_chunk_from_ptr(ptr);
if (tc->null_refs) {
tc->null_refs--;
- return -1;
+ goto err;
}
if (tc->refs) {
talloc_reference_destructor(tc->refs);
- return -1;
+ goto err;
}
if (tc->flags & TALLOC_FLAG_LOOP) {
/* we have a free loop - stop looping */
- return 0;
+ goto success;
}
if (tc->destructor) {
talloc_destructor_t d = tc->destructor;
if (d == (talloc_destructor_t)-1) {
- return -1;
+ goto err;
}
tc->destructor = (talloc_destructor_t)-1;
if (d(ptr) == -1) {
tc->destructor = d;
- return -1;
+ goto err;
}
tc->destructor = NULL;
}
@@ -594,10 +595,14 @@ int talloc_free(void *ptr)
tc->flags |= TALLOC_FLAG_FREE;
free(tc);
+ success:
+ errno = saved_errno;
return 0;
-}
-
+ err:
+ errno = saved_errno;
+ return -1;
+}
/*
A talloc version of realloc. The context argument is only used if
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 476d5c6d51..5a174b9881 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -771,9 +771,7 @@ struct node *read_node(struct connection *conn, const void *ctx,
return node;
error:
- err = errno;
talloc_free(node);
- errno = err;
return NULL;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 02/19] tools/xenstore: let tdb_logger() preserve errno
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
2022-12-13 16:00 ` [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 03/19] tools/xenstore: preserve errno across corrupt() Juergen Gross
` (17 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
tdb_logger() is called by TDB for logging errors. As errno is checked
often after doing the logging, tdb_logger() should preserve errno.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 5a174b9881..d48208ecfe 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2345,6 +2345,7 @@ static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
{
va_list ap;
char *s;
+ int saved_errno = errno;
va_start(ap, fmt);
s = talloc_vasprintf(NULL, fmt, ap);
@@ -2360,6 +2361,8 @@ static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
trace("talloc failure during logging\n");
syslog(LOG_ERR, "talloc failure during logging\n");
}
+
+ errno = saved_errno;
}
void setup_structure(bool live_update)
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 03/19] tools/xenstore: preserve errno across corrupt()
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
2022-12-13 16:00 ` [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno Juergen Gross
2022-12-13 16:00 ` [PATCH v2 02/19] tools/xenstore: let tdb_logger() " Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped Juergen Gross
` (16 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
Let corrupt() preserve errno in order to be able to simplify error
handling in future.
This is rather easy as the errno value when entering corrupt() is
saved already.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index d48208ecfe..8c2cca62b7 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2549,6 +2549,8 @@ void corrupt(struct connection *conn, const char *fmt, ...)
talloc_free(str);
check_store();
+
+ errno = saved_errno;
}
#ifndef NO_SOCKETS
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (2 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 03/19] tools/xenstore: preserve errno across corrupt() Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 19:01 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 05/19] tools/xenstore: enhance hashtable implementation Juergen Gross
` (15 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
When a domain has been released by Xen tools, remove all its
registered watches. This avoids sending watch events to the dead domain
when all the nodes related to it are being removed by the Xen tools.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- move call to do_release() (Julien Grall)
---
tools/xenstore/xenstored_domain.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index aa86892fed..e669c89e94 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -740,6 +740,9 @@ int do_release(const void *ctx, struct connection *conn,
if (IS_ERR(domain))
return -PTR_ERR(domain);
+ /* Avoid triggering watch events when the domain's nodes are deleted. */
+ conn_delete_all_watches(domain->conn);
+
talloc_free(domain->conn);
send_ack(conn, XS_RELEASE);
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 05/19] tools/xenstore: enhance hashtable implementation
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (3 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid Juergen Gross
` (14 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
Today it is possible to set a flag when calling hashtable_destroy() in
order to specify whether the data associated with the hashtable entries
should be freed or not. The keys of the entries will always be freed.
Change that by replacing the flag of hashtable_destroy() by two flags
for create_hashtable() which will specify whether the data and/or the
key of each entry should be freed or not.
This will enable users to have the key e.g. as part of the data.
Add a new function hashtable_iterate() to call a user specified
function for each entry in the hashtable.
Add new primes to the primetable in order to support smaller sizes of
the hashtable. The primes are selected according to:
https://planetmath.org/goodhashtableprimes
Update the URL in the source as the old one wasn't correct any longer.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
V2:
- add const to hashtable_iterate() callback (Julien Grall)
---
tools/xenstore/hashtable.c | 66 +++++++++++++++++++++++----------
tools/xenstore/hashtable.h | 35 +++++++++++++++--
tools/xenstore/xenstored_core.c | 7 ++--
3 files changed, 82 insertions(+), 26 deletions(-)
diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index 6ac336eff1..299549c51e 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -16,6 +16,7 @@ struct entry
struct hashtable {
unsigned int tablelength;
+ unsigned int flags;
struct entry **table;
unsigned int entrycount;
unsigned int loadlimit;
@@ -25,12 +26,11 @@ struct hashtable {
};
/*
-Credit for primes table: Aaron Krowne
- http://br.endernet.org/~akrowne/
- http://planetmath.org/encyclopedia/GoodHashTablePrimes.html
-*/
+ * Credit for primes table: Aaron Krowne
+ * https://planetmath.org/goodhashtableprimes
+ */
static const unsigned int primes[] = {
-53, 97, 193, 389,
+11, 23, 53, 97, 193, 389,
769, 1543, 3079, 6151,
12289, 24593, 49157, 98317,
196613, 393241, 786433, 1572869,
@@ -52,7 +52,8 @@ indexFor(unsigned int tablelength, unsigned int hashvalue) {
struct hashtable *
create_hashtable(unsigned int minsize,
unsigned int (*hashf) (void*),
- int (*eqf) (void*,void*))
+ int (*eqf) (void*,void*),
+ unsigned int flags)
{
struct hashtable *h;
unsigned int pindex, size = primes[0];
@@ -73,6 +74,7 @@ create_hashtable(unsigned int minsize,
goto err1;
h->tablelength = size;
+ h->flags = flags;
h->primeindex = pindex;
h->entrycount = 0;
h->hashfn = hashf;
@@ -235,7 +237,8 @@ hashtable_remove(struct hashtable *h, void *k)
*pE = e->next;
h->entrycount--;
v = e->v;
- free(e->k);
+ if (h->flags & HASHTABLE_FREE_KEY)
+ free(e->k);
free(e);
return v;
}
@@ -246,29 +249,52 @@ hashtable_remove(struct hashtable *h, void *k)
}
/*****************************************************************************/
-/* destroy */
-void
-hashtable_destroy(struct hashtable *h, int free_values)
+int
+hashtable_iterate(struct hashtable *h,
+ int (*func)(const void *k, void *v, void *arg), void *arg)
{
+ int ret;
unsigned int i;
struct entry *e, *f;
struct entry **table = h->table;
- if (free_values)
+
+ for (i = 0; i < h->tablelength; i++)
{
- for (i = 0; i < h->tablelength; i++)
+ e = table[i];
+ while (e)
{
- e = table[i];
- while (NULL != e)
- { f = e; e = e->next; free(f->k); free(f->v); free(f); }
+ f = e;
+ e = e->next;
+ ret = func(f->k, f->v, arg);
+ if (ret)
+ return ret;
}
}
- else
+
+ return 0;
+}
+
+/*****************************************************************************/
+/* destroy */
+void
+hashtable_destroy(struct hashtable *h)
+{
+ unsigned int i;
+ struct entry *e, *f;
+ struct entry **table = h->table;
+
+ for (i = 0; i < h->tablelength; i++)
{
- for (i = 0; i < h->tablelength; i++)
+ e = table[i];
+ while (NULL != e)
{
- e = table[i];
- while (NULL != e)
- { f = e; e = e->next; free(f->k); free(f); }
+ f = e;
+ e = e->next;
+ if (h->flags & HASHTABLE_FREE_KEY)
+ free(f->k);
+ if (h->flags & HASHTABLE_FREE_VALUE)
+ free(f->v);
+ free(f);
}
}
free(h->table);
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 62fef6081a..6d65431f96 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -12,13 +12,21 @@ struct hashtable;
* @param minsize minimum initial size of hashtable
* @param hashfunction function for hashing keys
* @param key_eq_fn function for determining key equality
+ * @param flags flags HASHTABLE_*
* @return newly created hashtable or NULL on failure
*/
+/* Let hashtable_destroy() free the entries' values. */
+#define HASHTABLE_FREE_VALUE (1U << 0)
+/* Let hashtable_remove() and hashtable_destroy() free the entries' keys. */
+#define HASHTABLE_FREE_KEY (1U << 1)
+
struct hashtable *
create_hashtable(unsigned int minsize,
unsigned int (*hashfunction) (void*),
- int (*key_eq_fn) (void*,void*));
+ int (*key_eq_fn) (void*,void*),
+ unsigned int flags
+);
/*****************************************************************************
* hashtable_insert
@@ -76,16 +84,37 @@ hashtable_remove(struct hashtable *h, void *k);
unsigned int
hashtable_count(struct hashtable *h);
+/*****************************************************************************
+ * hashtable_iterate
+
+ * @name hashtable_iterate
+ * @param h the hashtable
+ * @param func function to call for each entry
+ * @param arg user supplied parameter for func
+ * @return 0 if okay, non-zero return value of func (and iteration
+ * was aborted)
+ *
+ * Iterates over all entries in the hashtable and calls func with the
+ * key, value, and the user supplied parameter.
+ * func returning a non-zero value will abort the iteration. In case func is
+ * removing an entry other than itself from the hashtable, it must return a
+ * non-zero value in order to abort the iteration. Inserting entries is
+ * allowed, but it is undefined whether func will be called for those new
+ * entries during this iteration.
+ */
+int
+hashtable_iterate(struct hashtable *h,
+ int (*func)(const void *k, void *v, void *arg), void *arg);
+
/*****************************************************************************
* hashtable_destroy
* @name hashtable_destroy
* @param h the hashtable
- * @param free_values whether to call 'free' on the remaining values
*/
void
-hashtable_destroy(struct hashtable *h, int free_values);
+hashtable_destroy(struct hashtable *h);
#endif /* __HASHTABLE_CWC22_H__ */
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 8c2cca62b7..1650821922 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2512,7 +2512,9 @@ void check_store(void)
.enoent = check_store_enoent,
};
- reachable = create_hashtable(16, hash_from_key_fn, keys_equal_fn);
+ /* Don't free values (they are all void *1) */
+ reachable = create_hashtable(16, hash_from_key_fn, keys_equal_fn,
+ HASHTABLE_FREE_KEY);
if (!reachable) {
log("check_store: ENOMEM");
return;
@@ -2526,8 +2528,7 @@ void check_store(void)
clean_store(reachable);
log("Checking store complete.");
- hashtable_destroy(reachable, 0 /* Don't free values (they are all
- (void *)1) */);
+ hashtable_destroy(reachable);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (4 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 05/19] tools/xenstore: enhance hashtable implementation Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 19:09 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths Juergen Gross
` (13 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Today finding a struct domain by its domain id requires to scan the
list of domains until finding the correct domid.
Add a hashlist for being able to speed this up. This allows to remove
the linking of struct domain in a list. Note that the list of changed
domains per transaction is kept as a list, as there are no known use
cases with more than 4 domains being touched in a single transaction
(this would be a device handled by a driver domain and being assigned
to a HVM domain with device model in a stubdom, plus the control
domain).
Some simple performance tests comparing the scanning and hashlist have
shown that the hashlist will win as soon as more than 6 entries need
to be scanned.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add comment, fix return value of check_domain() (Julien Grall)
---
tools/xenstore/xenstored_domain.c | 102 ++++++++++++++++++------------
1 file changed, 60 insertions(+), 42 deletions(-)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index e669c89e94..3ad1028edb 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -48,8 +48,6 @@ static struct node_perms dom_introduce_perms;
struct domain
{
- struct list_head list;
-
/* The id of this domain */
unsigned int domid;
@@ -96,7 +94,7 @@ struct domain
bool wrl_delay_logged;
};
-static LIST_HEAD(domains);
+static struct hashtable *domhash;
static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
{
@@ -309,7 +307,7 @@ static int destroy_domain(void *_domain)
domain_tree_remove(domain);
- list_del(&domain->list);
+ hashtable_remove(domhash, &domain->domid);
if (!domain->introduced)
return 0;
@@ -341,43 +339,50 @@ static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo)
dominfo->domid == domid;
}
-void check_domains(void)
+static int check_domain(const void *k, void *v, void *arg)
{
xc_dominfo_t dominfo;
- struct domain *domain;
struct connection *conn;
- int notify = 0;
bool dom_valid;
+ struct domain *domain = v;
+ bool *notify = arg;
- again:
- list_for_each_entry(domain, &domains, list) {
- dom_valid = get_domain_info(domain->domid, &dominfo);
- if (!domain->introduced) {
- if (!dom_valid) {
- talloc_free(domain);
- goto again;
- }
- continue;
- }
- if (dom_valid) {
- if ((dominfo.crashed || dominfo.shutdown)
- && !domain->shutdown) {
- domain->shutdown = true;
- notify = 1;
- }
- if (!dominfo.dying)
- continue;
- }
- if (domain->conn) {
- /* domain is a talloc child of domain->conn. */
- conn = domain->conn;
- domain->conn = NULL;
- talloc_unlink(talloc_autofree_context(), conn);
- notify = 0; /* destroy_domain() fires the watch */
- goto again;
+ dom_valid = get_domain_info(domain->domid, &dominfo);
+ if (!domain->introduced) {
+ if (!dom_valid)
+ talloc_free(domain);
+ return 0;
+ }
+ if (dom_valid) {
+ if ((dominfo.crashed || dominfo.shutdown)
+ && !domain->shutdown) {
+ domain->shutdown = true;
+ *notify = true;
}
+ if (!dominfo.dying)
+ return 0;
+ }
+ if (domain->conn) {
+ /* domain is a talloc child of domain->conn. */
+ conn = domain->conn;
+ domain->conn = NULL;
+ talloc_unlink(talloc_autofree_context(), conn);
+ *notify = false; /* destroy_domain() fires the watch */
+
+ /* Above unlink might result in 2 domains being freed! */
+ return 1;
}
+ return 0;
+}
+
+void check_domains(void)
+{
+ bool notify = false;
+
+ while (hashtable_iterate(domhash, check_domain, ¬ify))
+ ;
+
if (notify)
fire_watches(NULL, NULL, "@releaseDomain", NULL, true, NULL);
}
@@ -415,13 +420,7 @@ static char *talloc_domain_path(const void *context, unsigned int domid)
static struct domain *find_domain_struct(unsigned int domid)
{
- struct domain *i;
-
- list_for_each_entry(i, &domains, list) {
- if (i->domid == domid)
- return i;
- }
- return NULL;
+ return hashtable_search(domhash, &domid);
}
int domain_get_quota(const void *ctx, struct connection *conn,
@@ -470,9 +469,13 @@ static struct domain *alloc_domain(const void *context, unsigned int domid)
domain->generation = generation;
domain->introduced = false;
- talloc_set_destructor(domain, destroy_domain);
+ if (!hashtable_insert(domhash, &domain->domid, domain)) {
+ talloc_free(domain);
+ errno = ENOMEM;
+ return NULL;
+ }
- list_add(&domain->list, &domains);
+ talloc_set_destructor(domain, destroy_domain);
return domain;
}
@@ -906,10 +909,25 @@ void dom0_init(void)
xenevtchn_notify(xce_handle, dom0->port);
}
+static unsigned int domhash_fn(void *k)
+{
+ return *(unsigned int *)k;
+}
+
+static int domeq_fn(void *key1, void *key2)
+{
+ return *(unsigned int *)key1 == *(unsigned int *)key2;
+}
+
void domain_init(int evtfd)
{
int rc;
+ /* Start with a random rather low domain count for the hashtable. */
+ domhash = create_hashtable(8, domhash_fn, domeq_fn, 0);
+ if (!domhash)
+ barf_perror("Failed to allocate domain hashtable");
+
xc_handle = talloc(talloc_autofree_context(), xc_interface*);
if (!xc_handle)
barf_perror("Failed to allocate domain handle");
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (5 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 19:39 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length Juergen Gross
` (12 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Instead of special casing the permission handling and watch event
firing for the special watch paths "@introduceDomain" and
"@releaseDomain", use static dummy nodes added to the data base when
starting Xenstore.
The node accounting needs to reflect that change by adding the special
nodes in the domain_entry_fix() call in setup_structure().
Note that this requires to rework the calls of fire_watches() for the
special events in order to avoid leaking memory.
Move the check for a valid node name from get_node() to
get_node_canonicalized(), as it allows to use get_node() for the
special nodes, too.
In order to avoid read and write accesses to the special nodes use a
special variant for obtaining the current node data for the permission
handling.
This allows to simplify quite some code. In future sub-nodes of the
special nodes will be possible due to this change, allowing more fine
grained permission control of special events for specific domains.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add get_spec_node()
- expand commit message (Julien Grall)
---
tools/xenstore/xenstored_control.c | 3 -
tools/xenstore/xenstored_core.c | 92 +++++++++-------
tools/xenstore/xenstored_domain.c | 162 ++++-------------------------
tools/xenstore/xenstored_domain.h | 6 --
tools/xenstore/xenstored_watch.c | 17 +--
5 files changed, 80 insertions(+), 200 deletions(-)
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index d1aaa00bf4..41e6992591 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -676,9 +676,6 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
if (ret)
goto out;
ret = dump_state_connections(fp);
- if (ret)
- goto out;
- ret = dump_state_special_nodes(fp);
if (ret)
goto out;
ret = dump_state_nodes(fp, ctx);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1650821922..f96714e1b8 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -616,7 +616,8 @@ static void get_acc_data(TDB_DATA *key, struct node_account_data *acc)
static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key,
unsigned int domid)
{
- return (!conn || key->dptr[0] == '/') ? domid : conn->id;
+ return (!conn || key->dptr[0] == '/' || key->dptr[0] == '@')
+ ? domid : conn->id;
}
int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data,
@@ -958,10 +959,6 @@ static struct node *get_node(struct connection *conn,
{
struct node *node;
- if (!name || !is_valid_nodename(name)) {
- errno = EINVAL;
- return NULL;
- }
node = read_node(conn, ctx, name);
/* If we don't have permission, we don't have node. */
if (node) {
@@ -1250,9 +1247,23 @@ static struct node *get_node_canonicalized(struct connection *conn,
*canonical_name = canonicalize(conn, ctx, name);
if (!*canonical_name)
return NULL;
+ if (!is_valid_nodename(*canonical_name)) {
+ errno = EINVAL;
+ return NULL;
+ }
return get_node(conn, ctx, *canonical_name, perm);
}
+static struct node *get_spec_node(struct connection *conn, const void *ctx,
+ const char *name, char **canonical_name,
+ unsigned int perm)
+{
+ if (name[0] == '@')
+ return get_node(conn, ctx, name, perm);
+
+ return get_node_canonicalized(conn, ctx, name, canonical_name, perm);
+}
+
static int send_directory(const void *ctx, struct connection *conn,
struct buffered_data *in)
{
@@ -1737,8 +1748,7 @@ static int do_get_perms(const void *ctx, struct connection *conn,
char *strings;
unsigned int len;
- node = get_node_canonicalized(conn, ctx, onearg(in), NULL,
- XS_PERM_READ);
+ node = get_spec_node(conn, ctx, onearg(in), NULL, XS_PERM_READ);
if (!node)
return errno;
@@ -1780,17 +1790,9 @@ static int do_set_perms(const void *ctx, struct connection *conn,
if (perms.p[0].perms & XS_PERM_IGNORE)
return ENOENT;
- /* First arg is node name. */
- if (strstarts(in->buffer, "@")) {
- if (set_perms_special(conn, in->buffer, &perms))
- return errno;
- send_ack(conn, XS_SET_PERMS);
- return 0;
- }
-
/* We must own node to do this (tools can do this too). */
- node = get_node_canonicalized(conn, ctx, in->buffer, &name,
- XS_PERM_WRITE | XS_PERM_OWNER);
+ node = get_spec_node(conn, ctx, in->buffer, &name,
+ XS_PERM_WRITE | XS_PERM_OWNER);
if (!node)
return errno;
@@ -2388,7 +2390,9 @@ void setup_structure(bool live_update)
manual_node("/", "tool");
manual_node("/tool", "xenstored");
manual_node("/tool/xenstored", NULL);
- domain_entry_fix(dom0_domid, 3, true);
+ manual_node("@releaseDomain", NULL);
+ manual_node("@introduceDomain", NULL);
+ domain_entry_fix(dom0_domid, 5, true);
}
check_store();
@@ -3170,6 +3174,23 @@ static int dump_state_node(const void *ctx, struct connection *conn,
return WALK_TREE_OK;
}
+static int dump_state_special_node(FILE *fp, const void *ctx,
+ struct dump_node_data *data,
+ const char *name)
+{
+ struct node *node;
+ int ret;
+
+ node = read_node(NULL, ctx, name);
+ if (!node)
+ return dump_state_node_err(data, "Dump node read node error");
+
+ ret = dump_state_node(ctx, NULL, node, data);
+ talloc_free(node);
+
+ return ret;
+}
+
const char *dump_state_nodes(FILE *fp, const void *ctx)
{
struct dump_node_data data = {
@@ -3181,6 +3202,11 @@ const char *dump_state_nodes(FILE *fp, const void *ctx)
if (walk_node_tree(ctx, NULL, "/", &walkfuncs, &data))
return data.err;
+ if (dump_state_special_node(fp, ctx, &data, "@releaseDomain"))
+ return data.err;
+ if (dump_state_special_node(fp, ctx, &data, "@introduceDomain"))
+ return data.err;
+
return NULL;
}
@@ -3354,25 +3380,21 @@ void read_state_node(const void *ctx, const void *state)
node->perms.p[i].id = sn->perms[i].domid;
}
- if (strstarts(name, "@")) {
- set_perms_special(&conn, name, &node->perms);
- talloc_free(node);
- return;
- }
-
- parentname = get_parent(node, name);
- if (!parentname)
- barf("allocation error restoring node");
- parent = read_node(NULL, node, parentname);
- if (!parent)
- barf("read parent error restoring node");
+ if (!strstarts(name, "@")) {
+ parentname = get_parent(node, name);
+ if (!parentname)
+ barf("allocation error restoring node");
+ parent = read_node(NULL, node, parentname);
+ if (!parent)
+ barf("read parent error restoring node");
- if (add_child(node, parent, name))
- barf("allocation error restoring node");
+ if (add_child(node, parent, name))
+ barf("allocation error restoring node");
- set_tdb_key(parentname, &key);
- if (write_node_raw(NULL, &key, parent, true))
- barf("write parent error restoring node");
+ set_tdb_key(parentname, &key);
+ if (write_node_raw(NULL, &key, parent, true))
+ barf("write parent error restoring node");
+ }
set_tdb_key(name, &key);
if (write_node_raw(NULL, &key, node, true))
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 3ad1028edb..c881c93d1d 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -43,9 +43,6 @@ static evtchn_port_t virq_port;
xenevtchn_handle *xce_handle = NULL;
-static struct node_perms dom_release_perms;
-static struct node_perms dom_introduce_perms;
-
struct domain
{
/* The id of this domain */
@@ -225,27 +222,6 @@ static void unmap_interface(void *interface)
xengnttab_unmap(*xgt_handle, interface, 1);
}
-static void remove_domid_from_perm(struct node_perms *perms,
- struct domain *domain)
-{
- unsigned int cur, new;
-
- if (perms->p[0].id == domain->domid)
- perms->p[0].id = priv_domid;
-
- for (cur = new = 1; cur < perms->num; cur++) {
- if (perms->p[cur].id == domain->domid)
- continue;
-
- if (new != cur)
- perms->p[new] = perms->p[cur];
-
- new++;
- }
-
- perms->num = new;
-}
-
static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
struct node *node, void *arg)
{
@@ -297,8 +273,24 @@ static void domain_tree_remove(struct domain *domain)
"error when looking for orphaned nodes\n");
}
- remove_domid_from_perm(&dom_release_perms, domain);
- remove_domid_from_perm(&dom_introduce_perms, domain);
+ walk_node_tree(domain, NULL, "@releaseDomain", &walkfuncs, domain);
+ walk_node_tree(domain, NULL, "@introduceDomain", &walkfuncs, domain);
+}
+
+static void fire_special_watches(const char *name)
+{
+ void *ctx = talloc_new(NULL);
+ struct node *node;
+
+ if (!ctx)
+ return;
+
+ node = read_node(NULL, ctx, name);
+
+ if (node)
+ fire_watches(NULL, ctx, name, node, true, NULL);
+
+ talloc_free(ctx);
}
static int destroy_domain(void *_domain)
@@ -326,7 +318,7 @@ static int destroy_domain(void *_domain)
unmap_interface(domain->interface);
}
- fire_watches(NULL, domain, "@releaseDomain", NULL, true, NULL);
+ fire_special_watches("@releaseDomain");
wrl_domain_destroy(domain);
@@ -384,7 +376,7 @@ void check_domains(void)
;
if (notify)
- fire_watches(NULL, NULL, "@releaseDomain", NULL, true, NULL);
+ fire_special_watches("@releaseDomain");
}
/* We scan all domains rather than use the information given here. */
@@ -633,8 +625,7 @@ static struct domain *introduce_domain(const void *ctx,
}
if (!is_master_domain && !restore)
- fire_watches(NULL, ctx, "@introduceDomain", NULL,
- true, NULL);
+ fire_special_watches("@introduceDomain");
} else {
/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
if (domain->port)
@@ -840,59 +831,6 @@ const char *get_implicit_path(const struct connection *conn)
return conn->domain->path;
}
-static int set_dom_perms_default(struct node_perms *perms)
-{
- perms->num = 1;
- perms->p = talloc_array(NULL, struct xs_permissions, perms->num);
- if (!perms->p)
- return -1;
- perms->p->id = 0;
- perms->p->perms = XS_PERM_NONE;
-
- return 0;
-}
-
-static struct node_perms *get_perms_special(const char *name)
-{
- if (!strcmp(name, "@releaseDomain"))
- return &dom_release_perms;
- if (!strcmp(name, "@introduceDomain"))
- return &dom_introduce_perms;
- return NULL;
-}
-
-int set_perms_special(struct connection *conn, const char *name,
- struct node_perms *perms)
-{
- struct node_perms *p;
-
- p = get_perms_special(name);
- if (!p)
- return EINVAL;
-
- if ((perm_for_conn(conn, p) & (XS_PERM_WRITE | XS_PERM_OWNER)) !=
- (XS_PERM_WRITE | XS_PERM_OWNER))
- return EACCES;
-
- p->num = perms->num;
- talloc_free(p->p);
- p->p = perms->p;
- talloc_steal(NULL, perms->p);
-
- return 0;
-}
-
-bool check_perms_special(const char *name, struct connection *conn)
-{
- struct node_perms *p;
-
- p = get_perms_special(name);
- if (!p)
- return false;
-
- return perm_for_conn(conn, p) & XS_PERM_READ;
-}
-
void dom0_init(void)
{
evtchn_port_t port;
@@ -962,10 +900,6 @@ void domain_init(int evtfd)
if (xce_handle == NULL)
barf_perror("Failed to open evtchn device");
- if (set_dom_perms_default(&dom_release_perms) ||
- set_dom_perms_default(&dom_introduce_perms))
- barf_perror("Failed to set special permissions");
-
if ((rc = xenevtchn_bind_virq(xce_handle, VIRQ_DOM_EXC)) == -1)
barf_perror("Failed to bind to domain exception virq port");
virq_port = rc;
@@ -1535,60 +1469,6 @@ const char *dump_state_connections(FILE *fp)
return ret;
}
-static const char *dump_state_special_node(FILE *fp, const char *name,
- const struct node_perms *perms)
-{
- struct xs_state_record_header head;
- struct xs_state_node sn;
- unsigned int pathlen;
- const char *ret;
-
- pathlen = strlen(name) + 1;
-
- head.type = XS_STATE_TYPE_NODE;
- head.length = sizeof(sn);
-
- sn.conn_id = 0;
- sn.ta_id = 0;
- sn.ta_access = 0;
- sn.perm_n = perms->num;
- sn.path_len = pathlen;
- sn.data_len = 0;
- head.length += perms->num * sizeof(*sn.perms);
- head.length += pathlen;
- head.length = ROUNDUP(head.length, 3);
- if (fwrite(&head, sizeof(head), 1, fp) != 1)
- return "Dump special node error";
- if (fwrite(&sn, sizeof(sn), 1, fp) != 1)
- return "Dump special node error";
-
- ret = dump_state_node_perms(fp, perms->p, perms->num);
- if (ret)
- return ret;
-
- if (fwrite(name, pathlen, 1, fp) != 1)
- return "Dump special node path error";
-
- ret = dump_state_align(fp);
-
- return ret;
-}
-
-const char *dump_state_special_nodes(FILE *fp)
-{
- const char *ret;
-
- ret = dump_state_special_node(fp, "@releaseDomain",
- &dom_release_perms);
- if (ret)
- return ret;
-
- ret = dump_state_special_node(fp, "@introduceDomain",
- &dom_introduce_perms);
-
- return ret;
-}
-
void read_state_connection(const void *ctx, const void *state)
{
const struct xs_state_connection *sc = state;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index b38c82991d..630641d620 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -99,11 +99,6 @@ void domain_outstanding_domid_dec(unsigned int domid);
int domain_get_quota(const void *ctx, struct connection *conn,
unsigned int domid);
-/* Special node permission handling. */
-int set_perms_special(struct connection *conn, const char *name,
- struct node_perms *perms);
-bool check_perms_special(const char *name, struct connection *conn);
-
/* Write rate limiting */
#define WRL_FACTOR 1000 /* for fixed-point arithmetic */
@@ -132,7 +127,6 @@ void wrl_apply_debit_direct(struct connection *conn);
void wrl_apply_debit_trans_commit(struct connection *conn);
const char *dump_state_connections(FILE *fp);
-const char *dump_state_special_nodes(FILE *fp);
void read_state_connection(const void *ctx, const void *state);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 316c08b7f7..75748ac109 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -46,13 +46,6 @@ struct watch
char *node;
};
-static bool check_special_event(const char *name)
-{
- assert(name);
-
- return strstarts(name, "@");
-}
-
/* Is child a subnode of parent, or equal? */
static bool is_child(const char *child, const char *parent)
{
@@ -153,14 +146,8 @@ void fire_watches(struct connection *conn, const void *ctx, const char *name,
/* Create an event for each watch. */
list_for_each_entry(i, &connections, list) {
- /* introduce/release domain watches */
- if (check_special_event(name)) {
- if (!check_perms_special(name, i))
- continue;
- } else {
- if (!watch_permitted(i, ctx, name, node, perms))
- continue;
- }
+ if (!watch_permitted(i, ctx, name, node, perms))
+ continue;
list_for_each_entry(watch, &i->watches, list) {
if (exact) {
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (6 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 19:42 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 09/19] tools/xenstore: move changed domain handling Juergen Gross
` (11 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Instead of storing a pointer to the path which is prepended to
relative paths in struct watch, just use the length of the prepended
path.
It should be noted that the now removed special case of the
relative path being "" in get_watch_path() can't happen at all.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- don't open code get_watch_path() (Julien Grall)
---
tools/xenstore/xenstored_watch.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 75748ac109..5c0f764781 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -39,8 +39,8 @@ struct watch
/* Current outstanding events applying to this watch. */
struct list_head events;
- /* Is this relative to connnection's implicit path? */
- const char *relative_path;
+ /* Offset into path for skipping prefix (used for relative paths). */
+ unsigned int prefix_len;
char *token;
char *node;
@@ -66,15 +66,7 @@ static bool is_child(const char *child, const char *parent)
static const char *get_watch_path(const struct watch *watch, const char *name)
{
- const char *path = name;
-
- if (watch->relative_path) {
- path += strlen(watch->relative_path);
- if (*path == '/') /* Could be "" */
- path++;
- }
-
- return path;
+ return name + watch->prefix_len;
}
/*
@@ -211,10 +203,7 @@ static struct watch *add_watch(struct connection *conn, char *path, char *token,
no_quota_check))
goto nomem;
- if (relative)
- watch->relative_path = get_implicit_path(conn);
- else
- watch->relative_path = NULL;
+ watch->prefix_len = relative ? strlen(get_implicit_path(conn)) + 1 : 0;
INIT_LIST_HEAD(&watch->events);
@@ -313,19 +302,19 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
unsigned int conn_id)
{
const char *ret = NULL;
+ const char *watch_path;
struct watch *watch;
struct xs_state_watch sw;
struct xs_state_record_header head;
- const char *path;
head.type = XS_STATE_TYPE_WATCH;
list_for_each_entry(watch, &conn->watches, list) {
head.length = sizeof(sw);
+ watch_path = get_watch_path(watch, watch->node);
sw.conn_id = conn_id;
- path = get_watch_path(watch, watch->node);
- sw.path_length = strlen(path) + 1;
+ sw.path_length = strlen(watch_path) + 1;
sw.token_length = strlen(watch->token) + 1;
head.length += sw.path_length + sw.token_length;
head.length = ROUNDUP(head.length, 3);
@@ -334,7 +323,7 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
if (fwrite(&sw, sizeof(sw), 1, fp) != 1)
return "Dump watch state error";
- if (fwrite(path, sw.path_length, 1, fp) != 1)
+ if (fwrite(watch_path, sw.path_length, 1, fp) != 1)
return "Dump watch path error";
if (fwrite(watch->token, sw.token_length, 1, fp) != 1)
return "Dump watch token error";
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 09/19] tools/xenstore: move changed domain handling
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (7 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 19:49 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface Juergen Gross
` (10 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Move all code related to struct changed_domain from
xenstored_transaction.c to xenstored_domain.c.
This will be needed later in order to simplify the accounting data
updates in cases of errors during a request.
Split the code to have a more generic base framework.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- remove unrelated change (Julien Grall)
---
tools/xenstore/xenstored_domain.c | 77 ++++++++++++++++++++++++++
tools/xenstore/xenstored_domain.h | 3 +
tools/xenstore/xenstored_transaction.c | 64 ++-------------------
3 files changed, 84 insertions(+), 60 deletions(-)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index c881c93d1d..3216119e83 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -91,6 +91,18 @@ struct domain
bool wrl_delay_logged;
};
+struct changed_domain
+{
+ /* List of all changed domains. */
+ struct list_head list;
+
+ /* Identifier of the changed domain. */
+ unsigned int domid;
+
+ /* Amount by which this domain's nbentry field has changed. */
+ int nbentry;
+};
+
static struct hashtable *domhash;
static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
@@ -541,6 +553,71 @@ static struct domain *find_domain_by_domid(unsigned int domid)
return (d && d->introduced) ? d : NULL;
}
+int acc_fix_domains(struct list_head *head, bool update)
+{
+ struct changed_domain *cd;
+ int cnt;
+
+ list_for_each_entry(cd, head, list) {
+ cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
+ if (!update) {
+ if (cnt >= quota_nb_entry_per_domain)
+ return ENOSPC;
+ if (cnt < 0)
+ return ENOMEM;
+ }
+ }
+
+ return 0;
+}
+
+static struct changed_domain *acc_find_changed_domain(struct list_head *head,
+ unsigned int domid)
+{
+ struct changed_domain *cd;
+
+ list_for_each_entry(cd, head, list) {
+ if (cd->domid == domid)
+ return cd;
+ }
+
+ return NULL;
+}
+
+static struct changed_domain *acc_get_changed_domain(const void *ctx,
+ struct list_head *head,
+ unsigned int domid)
+{
+ struct changed_domain *cd;
+
+ cd = acc_find_changed_domain(head, domid);
+ if (cd)
+ return cd;
+
+ cd = talloc_zero(ctx, struct changed_domain);
+ if (!cd)
+ return NULL;
+
+ cd->domid = domid;
+ list_add_tail(&cd->list, head);
+
+ return cd;
+}
+
+int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
+ unsigned int domid)
+{
+ struct changed_domain *cd;
+
+ cd = acc_get_changed_domain(ctx, head, domid);
+ if (!cd)
+ return errno;
+
+ cd->nbentry += val;
+
+ return 0;
+}
+
static void domain_conn_reset(struct domain *domain)
{
struct connection *conn = domain->conn;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 630641d620..9e20d2b17d 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -98,6 +98,9 @@ void domain_outstanding_dec(struct connection *conn);
void domain_outstanding_domid_dec(unsigned int domid);
int domain_get_quota(const void *ctx, struct connection *conn,
unsigned int domid);
+int acc_fix_domains(struct list_head *head, bool update);
+int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
+ unsigned int domid);
/* Write rate limiting */
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index ac854197ca..89b92f0baf 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -137,18 +137,6 @@ struct accessed_node
bool watch_exact;
};
-struct changed_domain
-{
- /* List of all changed domains in the context of this transaction. */
- struct list_head list;
-
- /* Identifier of the changed domain. */
- unsigned int domid;
-
- /* Amount by which this domain's nbentry field has changed. */
- int nbentry;
-};
-
struct transaction
{
/* List of all transactions active on this connection. */
@@ -514,24 +502,6 @@ int do_transaction_start(const void *ctx, struct connection *conn,
return 0;
}
-static int transaction_fix_domains(struct transaction *trans, bool update)
-{
- struct changed_domain *d;
- int cnt;
-
- list_for_each_entry(d, &trans->changed_domains, list) {
- cnt = domain_entry_fix(d->domid, d->nbentry, update);
- if (!update) {
- if (cnt >= quota_nb_entry_per_domain)
- return ENOSPC;
- if (cnt < 0)
- return ENOMEM;
- }
- }
-
- return 0;
-}
-
int do_transaction_end(const void *ctx, struct connection *conn,
struct buffered_data *in)
{
@@ -558,7 +528,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
if (streq(arg, "T")) {
if (trans->fail)
return ENOMEM;
- ret = transaction_fix_domains(trans, false);
+ ret = acc_fix_domains(&trans->changed_domains, false);
if (ret)
return ret;
ret = finalize_transaction(conn, trans, &is_corrupt);
@@ -568,7 +538,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
wrl_apply_debit_trans_commit(conn);
/* fix domain entry for each changed domain */
- transaction_fix_domains(trans, true);
+ acc_fix_domains(&trans->changed_domains, true);
if (is_corrupt)
corrupt(conn, "transaction inconsistency");
@@ -580,44 +550,18 @@ int do_transaction_end(const void *ctx, struct connection *conn,
void transaction_entry_inc(struct transaction *trans, unsigned int domid)
{
- struct changed_domain *d;
-
- list_for_each_entry(d, &trans->changed_domains, list)
- if (d->domid == domid) {
- d->nbentry++;
- return;
- }
-
- d = talloc(trans, struct changed_domain);
- if (!d) {
+ if (acc_add_dom_nbentry(trans, &trans->changed_domains, 1, domid)) {
/* Let the transaction fail. */
trans->fail = true;
- return;
}
- d->domid = domid;
- d->nbentry = 1;
- list_add_tail(&d->list, &trans->changed_domains);
}
void transaction_entry_dec(struct transaction *trans, unsigned int domid)
{
- struct changed_domain *d;
-
- list_for_each_entry(d, &trans->changed_domains, list)
- if (d->domid == domid) {
- d->nbentry--;
- return;
- }
-
- d = talloc(trans, struct changed_domain);
- if (!d) {
+ if (acc_add_dom_nbentry(trans, &trans->changed_domains, -1, domid)) {
/* Let the transaction fail. */
trans->fail = true;
- return;
}
- d->domid = domid;
- d->nbentry = -1;
- list_add_tail(&d->list, &trans->changed_domains);
}
void fail_transaction(struct transaction *trans)
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (8 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 09/19] tools/xenstore: move changed domain handling Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 20:15 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction Juergen Gross
` (9 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Rework the interface and the internals of the per-domain node
accounting:
- rename the functions to domain_nbentry_*() in order to better match
the related counter name
- switch from node pointer to domid as interface, as all nodes have the
owner filled in
- use a common internal function for adding a value to the counter
For the transaction case add a helper function to get the list head
of the per-transaction changed domains, enabling to eliminate the
transaction_entry_*() functions.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstore/xenstored_core.c | 22 ++---
tools/xenstore/xenstored_domain.c | 122 +++++++++++--------------
tools/xenstore/xenstored_domain.h | 10 +-
tools/xenstore/xenstored_transaction.c | 15 +--
tools/xenstore/xenstored_transaction.h | 7 +-
5 files changed, 72 insertions(+), 104 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f96714e1b8..61569cecbb 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, struct node *node)
static int destroy_node(struct connection *conn, struct node *node)
{
destroy_node_rm(conn, node);
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
/*
* It is not possible to easily revert the changes in a transaction.
@@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
for (i = node; i; i = i->parent) {
/* i->parent is set for each new node, so check quota. */
if (i->parent &&
- domain_entry(conn) >= quota_nb_entry_per_domain) {
+ domain_nbentry(conn) >= quota_nb_entry_per_domain) {
ret = ENOSPC;
goto err;
}
@@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
/* Account for new node */
if (i->parent) {
- if (domain_entry_inc(conn, i)) {
+ if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
destroy_node_rm(conn, i);
return NULL;
}
@@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection *conn,
watch_exact = strcmp(root, node->name);
fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
return WALK_TREE_RM_CHILDENTRY;
}
@@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct connection *conn,
return EPERM;
old_perms = node->perms;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = perms;
- if (domain_entry_inc(conn, node)) {
+ if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
node->perms = old_perms;
/*
* This should never fail because we had a reference on the
* domain before and Xenstored is single-threaded.
*/
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
return ENOMEM;
}
if (write_node(conn, node, false)) {
int saved_errno = errno;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = old_perms;
/* No failure possible as above. */
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
errno = saved_errno;
return errno;
@@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
manual_node("/tool/xenstored", NULL);
manual_node("@releaseDomain", NULL);
manual_node("@introduceDomain", NULL);
- domain_entry_fix(dom0_domid, 5, true);
+ domain_nbentry_fix(dom0_domid, 5, true);
}
check_store();
@@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
if (write_node_raw(NULL, &key, node, true))
barf("write node error restoring node");
- if (domain_entry_inc(&conn, node))
+ if (domain_nbentry_inc(&conn, node->perms.p[0].id))
barf("node accounting error restoring node");
talloc_free(node);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 3216119e83..40b24056c5 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
domain->nbentry--;
node->perms.p[0].id = priv_domid;
node->acc.memory = 0;
- domain_entry_inc(NULL, node);
+ domain_nbentry_inc(NULL, priv_domid);
if (write_node_raw(NULL, &key, node, true)) {
/* That's unfortunate. We only can try to continue. */
syslog(LOG_ERR,
@@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update)
int cnt;
list_for_each_entry(cd, head, list) {
- cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
+ cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
if (!update) {
if (cnt >= quota_nb_entry_per_domain)
return ENOSPC;
@@ -604,18 +604,19 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx,
return cd;
}
-int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
- unsigned int domid)
+static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
+ unsigned int domid)
{
struct changed_domain *cd;
cd = acc_get_changed_domain(ctx, head, domid);
if (!cd)
- return errno;
+ return 0;
+ errno = 0;
cd->nbentry += val;
- return 0;
+ return cd->nbentry;
}
static void domain_conn_reset(struct domain *domain)
@@ -988,30 +989,6 @@ void domain_deinit(void)
xenevtchn_unbind(xce_handle, virq_port);
}
-int domain_entry_inc(struct connection *conn, struct node *node)
-{
- struct domain *d;
- unsigned int domid;
-
- if (!node->perms.p)
- return 0;
-
- domid = node->perms.p[0].id;
-
- if (conn && conn->transaction) {
- transaction_entry_inc(conn->transaction, domid);
- } else {
- d = (conn && domid == conn->id && conn->domain) ? conn->domain
- : find_or_alloc_existing_domain(domid);
- if (d)
- d->nbentry++;
- else
- return ENOMEM;
- }
-
- return 0;
-}
-
/*
* Check whether a domain was created before or after a specific generation
* count (used for testing whether a node permission is older than a domain).
@@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node)
return 0;
}
-void domain_entry_dec(struct connection *conn, struct node *node)
+static int domain_nbentry_add(struct connection *conn, unsigned int domid,
+ int add, bool dom_exists)
{
struct domain *d;
- unsigned int domid;
-
- if (!node->perms.p)
- return;
+ struct list_head *head;
+ int ret;
- domid = node->perms.p ? node->perms.p[0].id : conn->id;
+ if (conn && domid == conn->id && conn->domain)
+ d = conn->domain;
+ else if (dom_exists) {
+ d = find_domain_struct(domid);
+ if (!d) {
+ errno = ENOENT;
+ corrupt(conn, "Missing domain %u\n", domid);
+ return -1;
+ }
+ } else {
+ d = find_or_alloc_existing_domain(domid);
+ if (!d) {
+ errno = ENOMEM;
+ return -1;
+ }
+ }
if (conn && conn->transaction) {
- transaction_entry_dec(conn->transaction, domid);
- } else {
- d = (conn && domid == conn->id && conn->domain) ? conn->domain
- : find_domain_struct(domid);
- if (d) {
- d->nbentry--;
- } else {
- errno = ENOENT;
- corrupt(conn,
- "Node \"%s\" owned by non-existing domain %u\n",
- node->name, domid);
+ head = transaction_get_changed_domains(conn->transaction);
+ ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
+ if (errno) {
+ fail_transaction(conn->transaction);
+ return -1;
}
+ return d->nbentry + ret;
}
+
+ d->nbentry += add;
+
+ return d->nbentry;
}
-int domain_entry_fix(unsigned int domid, int num, bool update)
+int domain_nbentry_inc(struct connection *conn, unsigned int domid)
{
- struct domain *d;
- int cnt;
+ return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
+}
- if (update) {
- d = find_domain_struct(domid);
- assert(d);
- } else {
- /*
- * We are called first with update == false in order to catch
- * any error. So do a possible allocation and check for error
- * only in this case, as in the case of update == true nothing
- * can go wrong anymore as the allocation already happened.
- */
- d = find_or_alloc_existing_domain(domid);
- if (!d)
- return -1;
- }
+int domain_nbentry_dec(struct connection *conn, unsigned int domid)
+{
+ return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
+}
- cnt = d->nbentry + num;
- assert(cnt >= 0);
+int domain_nbentry_fix(unsigned int domid, int num, bool update)
+{
+ int ret;
- if (update)
- d->nbentry = cnt;
+ ret = domain_nbentry_add(NULL, domid, update ? num : 0, update);
+ if (ret < 0 || update)
+ return ret;
- return domid_is_unprivileged(domid) ? cnt : 0;
+ return domid_is_unprivileged(domid) ? ret + num : 0;
}
-int domain_entry(struct connection *conn)
+int domain_nbentry(struct connection *conn)
{
return (domain_is_unprivileged(conn))
? conn->domain->nbentry
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 9e20d2b17d..1e402f2609 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -66,10 +66,10 @@ int domain_adjust_node_perms(struct node *node);
int domain_alloc_permrefs(struct node_perms *perms);
/* Quota manipulation */
-int domain_entry_inc(struct connection *conn, struct node *);
-void domain_entry_dec(struct connection *conn, struct node *);
-int domain_entry_fix(unsigned int domid, int num, bool update);
-int domain_entry(struct connection *conn);
+int domain_nbentry_inc(struct connection *conn, unsigned int domid);
+int domain_nbentry_dec(struct connection *conn, unsigned int domid);
+int domain_nbentry_fix(unsigned int domid, int num, bool update);
+int domain_nbentry(struct connection *conn);
int domain_memory_add(unsigned int domid, int mem, bool no_quota_check);
/*
@@ -99,8 +99,6 @@ void domain_outstanding_domid_dec(unsigned int domid);
int domain_get_quota(const void *ctx, struct connection *conn,
unsigned int domid);
int acc_fix_domains(struct list_head *head, bool update);
-int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
- unsigned int domid);
/* Write rate limiting */
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 89b92f0baf..82e5e66c18 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -548,20 +548,9 @@ int do_transaction_end(const void *ctx, struct connection *conn,
return 0;
}
-void transaction_entry_inc(struct transaction *trans, unsigned int domid)
+struct list_head *transaction_get_changed_domains(struct transaction *trans)
{
- if (acc_add_dom_nbentry(trans, &trans->changed_domains, 1, domid)) {
- /* Let the transaction fail. */
- trans->fail = true;
- }
-}
-
-void transaction_entry_dec(struct transaction *trans, unsigned int domid)
-{
- if (acc_add_dom_nbentry(trans, &trans->changed_domains, -1, domid)) {
- /* Let the transaction fail. */
- trans->fail = true;
- }
+ return &trans->changed_domains;
}
void fail_transaction(struct transaction *trans)
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index 3417303f94..b6f8cb7d0a 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -36,10 +36,6 @@ int do_transaction_end(const void *ctx, struct connection *conn,
struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
-/* inc/dec entry number local to trans while changing a node */
-void transaction_entry_inc(struct transaction *trans, unsigned int domid);
-void transaction_entry_dec(struct transaction *trans, unsigned int domid);
-
/* This node was accessed. */
int __must_check access_node(struct connection *conn, struct node *node,
enum node_access_type type, TDB_DATA *key);
@@ -54,6 +50,9 @@ void transaction_prepend(struct connection *conn, const char *name,
/* Mark the transaction as failed. This will prevent it to be committed. */
void fail_transaction(struct transaction *trans);
+/* Get the list head of the changed domains. */
+struct list_head *transaction_get_changed_domains(struct transaction *trans);
+
void conn_delete_all_transactions(struct connection *conn);
int check_transactions(struct hashtable *hash);
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (9 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 20:18 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 12/19] tools/xenstore: replace literal domid 0 with dom0_domid Juergen Gross
` (8 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
The accounting for the number of nodes of a domain in an active
transaction is not working correctly, as it allows to create arbitrary
number of nodes. The transaction will finally fail due to exceeding
the number of nodes quota, but before closing the transaction an
unprivileged guest could cause Xenstore to use a lot of memory.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstore/xenstored_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 40b24056c5..1ae79b5b54 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1118,9 +1118,8 @@ int domain_nbentry_fix(unsigned int domid, int num, bool update)
int domain_nbentry(struct connection *conn)
{
- return (domain_is_unprivileged(conn))
- ? conn->domain->nbentry
- : 0;
+ return domain_is_unprivileged(conn)
+ ? domain_nbentry_add(conn, conn->id, 0, true) : 0;
}
static bool domain_chk_quota(struct domain *domain, int mem)
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 12/19] tools/xenstore: replace literal domid 0 with dom0_domid
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (10 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 13/19] tools/xenstore: make domain_is_unprivileged() an inline function Juergen Gross
` (7 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
There are some places left where dom0 is associated with domid 0.
Use dom0_domid instead.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.c | 5 +++--
tools/xenstore/xenstored_domain.c | 8 ++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 61569cecbb..6c9d22b8a2 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2311,9 +2311,10 @@ static void accept_connection(int sock)
return;
conn = new_connection(&socket_funcs);
- if (conn)
+ if (conn) {
conn->fd = fd;
- else
+ conn->id = dom0_domid;
+ } else
close(fd);
}
#endif
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 1ae79b5b54..1e9d7545b7 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -324,7 +324,7 @@ static int destroy_domain(void *_domain)
if (domain->interface) {
/* Domain 0 was mapped by dom0_init, so it must be unmapped
using munmap() and not the grant unmap call. */
- if (domain->domid == 0)
+ if (domain->domid == dom0_domid)
unmap_xenbus(domain->interface);
else
unmap_interface(domain->interface);
@@ -408,7 +408,7 @@ void handle_event(void)
static bool domid_is_unprivileged(unsigned int domid)
{
- return domid != 0 && domid != priv_domid;
+ return domid != dom0_domid && domid != priv_domid;
}
bool domain_is_unprivileged(struct connection *conn)
@@ -796,7 +796,7 @@ static struct domain *onearg_domain(struct connection *conn,
return ERR_PTR(-EINVAL);
domid = atoi(domid_str);
- if (!domid)
+ if (domid == dom0_domid)
return ERR_PTR(-EINVAL);
return find_connected_domain(domid);
@@ -1002,7 +1002,7 @@ static int chk_domain_generation(unsigned int domid, uint64_t gen)
{
struct domain *d;
- if (!xc_handle && domid == 0)
+ if (!xc_handle && domid == dom0_domid)
return 1;
d = find_domain_struct(domid);
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 13/19] tools/xenstore: make domain_is_unprivileged() an inline function
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (11 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 12/19] tools/xenstore: replace literal domid 0 with dom0_domid Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 14/19] tools/xenstore: let chk_domain_generation() return a bool Juergen Gross
` (6 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
clang 14 is complaining about a NULL dereference for constructs like:
domain_is_unprivileged(conn) ? conn->in : NULL
as it can't know that domain_is_unprivileged(conn) will return false
if conn is NULL.
Fix that by making domain_is_unprivileged() an inline function (and
related to that domid_is_unprivileged(), too).
In order not having to make struct domain public, use conn->id instead
of conn->domain->domid for the test.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.h | 10 ++++++++++
tools/xenstore/xenstored_domain.c | 11 -----------
tools/xenstore/xenstored_domain.h | 2 --
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 37006d508d..3c4e27d0dd 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -297,6 +297,16 @@ void unmap_xenbus(void *interface);
static inline int xenbus_master_domid(void) { return dom0_domid; }
+static inline bool domid_is_unprivileged(unsigned int domid)
+{
+ return domid != dom0_domid && domid != priv_domid;
+}
+
+static inline bool domain_is_unprivileged(const struct connection *conn)
+{
+ return conn && domid_is_unprivileged(conn->id);
+}
+
/* Return the event channel used by xenbus. */
evtchn_port_t xenbus_evtchn(void);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 1e9d7545b7..a3ecdb382f 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -406,17 +406,6 @@ void handle_event(void)
barf_perror("Failed to write to event fd");
}
-static bool domid_is_unprivileged(unsigned int domid)
-{
- return domid != dom0_domid && domid != priv_domid;
-}
-
-bool domain_is_unprivileged(struct connection *conn)
-{
- return conn && conn->domain &&
- domid_is_unprivileged(conn->domain->domid);
-}
-
static char *talloc_domain_path(const void *context, unsigned int domid)
{
return talloc_asprintf(context, "/local/domain/%u", domid);
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 1e402f2609..22996e2576 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -59,8 +59,6 @@ void ignore_connection(struct connection *conn, unsigned int err);
/* Returns the implicit path of a connection (only domains have this) */
const char *get_implicit_path(const struct connection *conn);
-bool domain_is_unprivileged(struct connection *conn);
-
/* Remove node permissions for no longer existing domains. */
int domain_adjust_node_perms(struct node *node);
int domain_alloc_permrefs(struct node_perms *perms);
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 14/19] tools/xenstore: let chk_domain_generation() return a bool
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (12 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 13/19] tools/xenstore: make domain_is_unprivileged() an inline function Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework Juergen Gross
` (5 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
Instead of returning 0 or 1 let chk_domain_generation() return a
boolean value.
Simplify the only caller by removing the ret variable.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_domain.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index a3ecdb382f..65e94e3822 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -983,20 +983,20 @@ void domain_deinit(void)
* count (used for testing whether a node permission is older than a domain).
*
* Return values:
- * 0: domain has higher generation count (it is younger than a node with the
- * given count), or domain isn't existing any longer
- * 1: domain is older than the node
+ * false: domain has higher generation count (it is younger than a node with
+ * the given count), or domain isn't existing any longer
+ * true: domain is older than the node
*/
-static int chk_domain_generation(unsigned int domid, uint64_t gen)
+static bool chk_domain_generation(unsigned int domid, uint64_t gen)
{
struct domain *d;
if (!xc_handle && domid == dom0_domid)
- return 1;
+ return true;
d = find_domain_struct(domid);
- return (d && d->generation <= gen) ? 1 : 0;
+ return d && d->generation <= gen;
}
/*
@@ -1031,14 +1031,12 @@ int domain_alloc_permrefs(struct node_perms *perms)
int domain_adjust_node_perms(struct node *node)
{
unsigned int i;
- int ret;
for (i = 1; i < node->perms.num; i++) {
if (node->perms.p[i].perms & XS_PERM_IGNORE)
continue;
- ret = chk_domain_generation(node->perms.p[i].id,
- node->generation);
- if (!ret)
+ if (!chk_domain_generation(node->perms.p[i].id,
+ node->generation))
node->perms.p[i].perms |= XS_PERM_IGNORE;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (13 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 14/19] tools/xenstore: let chk_domain_generation() return a bool Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-20 21:50 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 16/19] tools/xenstore: make log macro globally available Juergen Gross
` (4 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Instead of using malloc() and friends, let the hashtable implementation
use the talloc framework.
This is more consistent with the rest of xenstored and it allows to
track memory usage via "xenstore-control memreport".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstore/hashtable.c | 86 ++++++++++++-------------------
tools/xenstore/hashtable.h | 3 +-
tools/xenstore/xenstored_core.c | 5 +-
tools/xenstore/xenstored_domain.c | 2 +-
4 files changed, 39 insertions(+), 57 deletions(-)
diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index 299549c51e..3b6745b692 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -6,6 +6,8 @@
#include <string.h>
#include <math.h>
#include <stdint.h>
+#include <stdarg.h>
+#include "talloc.h"
struct entry
{
@@ -50,7 +52,7 @@ indexFor(unsigned int tablelength, unsigned int hashvalue) {
/*****************************************************************************/
struct hashtable *
-create_hashtable(unsigned int minsize,
+create_hashtable(const void *ctx, unsigned int minsize,
unsigned int (*hashf) (void*),
int (*eqf) (void*,void*),
unsigned int flags)
@@ -66,10 +68,10 @@ create_hashtable(unsigned int minsize,
if (primes[pindex] > minsize) { size = primes[pindex]; break; }
}
- h = (struct hashtable *)calloc(1, sizeof(struct hashtable));
+ h = talloc_zero(ctx, struct hashtable);
if (NULL == h)
goto err0;
- h->table = (struct entry **)calloc(size, sizeof(struct entry *));
+ h->table = talloc_zero_array(h, struct entry *, size);
if (NULL == h->table)
goto err1;
@@ -83,7 +85,7 @@ create_hashtable(unsigned int minsize,
return h;
err1:
- free(h);
+ talloc_free(h);
err0:
return NULL;
}
@@ -115,47 +117,32 @@ hashtable_expand(struct hashtable *h)
if (h->primeindex == (prime_table_length - 1)) return 0;
newsize = primes[++(h->primeindex)];
- newtable = (struct entry **)calloc(newsize, sizeof(struct entry*));
- if (NULL != newtable)
+ newtable = talloc_realloc(h, h->table, struct entry *, newsize);
+ if (!newtable)
{
- /* This algorithm is not 'stable'. ie. it reverses the list
- * when it transfers entries between the tables */
- for (i = 0; i < h->tablelength; i++) {
- while (NULL != (e = h->table[i])) {
- h->table[i] = e->next;
- index = indexFor(newsize,e->h);
+ h->primeindex--;
+ return 0;
+ }
+
+ h->table = newtable;
+ memset(newtable + h->tablelength, 0,
+ (newsize - h->tablelength) * sizeof(*newtable));
+ for (i = 0; i < h->tablelength; i++) {
+ for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
+ index = indexFor(newsize,e->h);
+ if (index == i)
+ {
+ pE = &(e->next);
+ }
+ else
+ {
+ *pE = e->next;
e->next = newtable[index];
newtable[index] = e;
}
}
- free(h->table);
- h->table = newtable;
- }
- /* Plan B: realloc instead */
- else
- {
- newtable = (struct entry **)
- realloc(h->table, newsize * sizeof(struct entry *));
- if (NULL == newtable) { (h->primeindex)--; return 0; }
- h->table = newtable;
- memset(newtable + h->tablelength, 0,
- (newsize - h->tablelength) * sizeof(*newtable));
- for (i = 0; i < h->tablelength; i++) {
- for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
- index = indexFor(newsize,e->h);
- if (index == i)
- {
- pE = &(e->next);
- }
- else
- {
- *pE = e->next;
- e->next = newtable[index];
- newtable[index] = e;
- }
- }
- }
}
+
h->tablelength = newsize;
h->loadlimit = (unsigned int)
(((uint64_t)newsize * max_load_factor) / 100);
@@ -184,7 +171,7 @@ hashtable_insert(struct hashtable *h, void *k, void *v)
* element may be ok. Next time we insert, we'll try expanding again.*/
hashtable_expand(h);
}
- e = (struct entry *)calloc(1, sizeof(struct entry));
+ e = talloc_zero(h, struct entry);
if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
e->h = hash(h,k);
index = indexFor(h->tablelength,e->h);
@@ -238,8 +225,8 @@ hashtable_remove(struct hashtable *h, void *k)
h->entrycount--;
v = e->v;
if (h->flags & HASHTABLE_FREE_KEY)
- free(e->k);
- free(e);
+ talloc_free(e->k);
+ talloc_free(e);
return v;
}
pE = &(e->next);
@@ -280,25 +267,20 @@ void
hashtable_destroy(struct hashtable *h)
{
unsigned int i;
- struct entry *e, *f;
+ struct entry *e;
struct entry **table = h->table;
for (i = 0; i < h->tablelength; i++)
{
- e = table[i];
- while (NULL != e)
+ for (e = table[i]; e; e = e->next)
{
- f = e;
- e = e->next;
if (h->flags & HASHTABLE_FREE_KEY)
- free(f->k);
+ talloc_free(e->k);
if (h->flags & HASHTABLE_FREE_VALUE)
- free(f->v);
- free(f);
+ talloc_free(e->v);
}
}
- free(h->table);
- free(h);
+ talloc_free(h);
}
/*
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 6d65431f96..becec73092 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -9,6 +9,7 @@ struct hashtable;
* create_hashtable
* @name create_hashtable
+ * @param ctx talloc context to use for allocations
* @param minsize minimum initial size of hashtable
* @param hashfunction function for hashing keys
* @param key_eq_fn function for determining key equality
@@ -22,7 +23,7 @@ struct hashtable;
#define HASHTABLE_FREE_KEY (1U << 1)
struct hashtable *
-create_hashtable(unsigned int minsize,
+create_hashtable(const void *ctx, unsigned int minsize,
unsigned int (*hashfunction) (void*),
int (*key_eq_fn) (void*,void*),
unsigned int flags
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 6c9d22b8a2..60433265ed 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2419,11 +2419,10 @@ static int keys_equal_fn(void *key1, void *key2)
int remember_string(struct hashtable *hash, const char *str)
{
- char *k = malloc(strlen(str) + 1);
+ char *k = talloc_strdup(NULL, str);
if (!k)
return 0;
- strcpy(k, str);
return hashtable_insert(hash, k, (void *)1);
}
@@ -2518,7 +2517,7 @@ void check_store(void)
};
/* Don't free values (they are all void *1) */
- reachable = create_hashtable(16, hash_from_key_fn, keys_equal_fn,
+ reachable = create_hashtable(NULL, 16, hash_from_key_fn, keys_equal_fn,
HASHTABLE_FREE_KEY);
if (!reachable) {
log("check_store: ENOMEM");
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 65e94e3822..4d735a5951 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -929,7 +929,7 @@ void domain_init(int evtfd)
int rc;
/* Start with a random rather low domain count for the hashtable. */
- domhash = create_hashtable(8, domhash_fn, domeq_fn, 0);
+ domhash = create_hashtable(NULL, 8, domhash_fn, domeq_fn, 0);
if (!domhash)
barf_perror("Failed to allocate domain hashtable");
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 16/19] tools/xenstore: make log macro globally available
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (14 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 17/19] tools/xenstore: introduce trace classes Juergen Gross
` (3 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD, Julien Grall
Move the definition of the log() macro to xenstored_core.h in order
to make it usable from other source files, too.
While at it preserve errno from being modified.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
tools/xenstore/xenstored_core.c | 14 --------------
tools/xenstore/xenstored_core.h | 15 +++++++++++++++
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 60433265ed..48c2793aeb 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -88,20 +88,6 @@ TDB_CONTEXT *tdb_ctx = NULL;
static const char *sockmsg_string(enum xsd_sockmsg_type type);
-#define log(...) \
- do { \
- char *s = talloc_asprintf(NULL, __VA_ARGS__); \
- if (s) { \
- trace("%s\n", s); \
- syslog(LOG_ERR, "%s\n", s); \
- talloc_free(s); \
- } else { \
- trace("talloc failure during logging\n"); \
- syslog(LOG_ERR, "talloc failure during logging\n"); \
- } \
- } while (0)
-
-
int quota_nb_entry_per_domain = 1000;
int quota_nb_watch_per_domain = 128;
int quota_max_entry_size = 2048; /* 2K */
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3c4e27d0dd..3b96ecd018 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -267,6 +267,21 @@ void trace(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
void reopen_log(void);
void close_log(void);
+#define log(...) \
+ do { \
+ int _saved_errno = errno; \
+ char *s = talloc_asprintf(NULL, __VA_ARGS__); \
+ if (s) { \
+ trace("%s\n", s); \
+ syslog(LOG_ERR, "%s\n", s); \
+ talloc_free(s); \
+ } else { \
+ trace("talloc failure during logging\n"); \
+ syslog(LOG_ERR, "talloc failure during logging\n"); \
+ } \
+ errno = _saved_errno; \
+ } while (0)
+
extern int orig_argc;
extern char **orig_argv;
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 17/19] tools/xenstore: introduce trace classes
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (15 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 16/19] tools/xenstore: make log macro globally available Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 18/19] tools/xenstore: let check_store() check the accounting data Juergen Gross
` (2 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Anthony PERARD
Make the xenstored internal trace configurable by adding classes
which can be switched on and off independently from each other.
Define the following classes:
- obj: Creation and deletion of interesting "objects" (watch,
transaction, connection)
- io: incoming requests and outgoing responses
- wrl: write limiting
Per default "obj" and "io" are switched on.
Entries written via trace() will always be printed (if tracing is on
at all).
Add the capability to control the trace settings via the "log"
command and via a new "--log-control" command line option.
Add a missing trace_create() call for creating a transaction.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- keep "log" and "logfile" command names (Julien Grall)
---
docs/misc/xenstore.txt | 10 +++--
tools/xenstore/xenstored_control.c | 31 +++++++++++++--
tools/xenstore/xenstored_core.c | 55 ++++++++++++++++++++++++--
tools/xenstore/xenstored_core.h | 6 +++
tools/xenstore/xenstored_domain.c | 27 +++++++------
tools/xenstore/xenstored_transaction.c | 1 +
6 files changed, 106 insertions(+), 24 deletions(-)
diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
index 44428ae3a7..8887e7df88 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -409,10 +409,12 @@ CONTROL <command>|[<parameters>|]
error string in case of failure. -s can return "BUSY" in case
of an active transaction, a retry of -s can be done in that
case.
- log|on
- turn xenstore logging on
- log|off
- turn xenstore logging off
+ log|[on|off|+<switch>|-<switch>]
+ without parameters: show possible log switches
+ on: turn xenstore logging on
+ off: turn xenstore logging off
+ +<switch>: activates log entries for <switch>,
+ -<switch>: deactivates log entries for <switch>
logfile|<file-name>
log to specified file
memreport|[<file-name>]
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 41e6992591..000b2bb8c7 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -182,6 +182,28 @@ static int do_control_check(const void *ctx, struct connection *conn,
static int do_control_log(const void *ctx, struct connection *conn,
char **vec, int num)
{
+ int ret;
+
+ if (num == 0) {
+ char *resp = talloc_asprintf(ctx, "Log switch settings:\n");
+ unsigned int idx;
+ bool on;
+
+ if (!resp)
+ return ENOMEM;
+ for (idx = 0; trace_switches[idx]; idx++) {
+ on = trace_flags & (1u << idx);
+ resp = talloc_asprintf_append(resp, "%-8s: %s\n",
+ trace_switches[idx],
+ on ? "on" : "off");
+ if (!resp)
+ return ENOMEM;
+ }
+
+ send_reply(conn, XS_CONTROL, resp, strlen(resp) + 1);
+ return 0;
+ }
+
if (num != 1)
return EINVAL;
@@ -189,8 +211,11 @@ static int do_control_log(const void *ctx, struct connection *conn,
reopen_log();
else if (!strcmp(vec[0], "off"))
close_log();
- else
- return EINVAL;
+ else {
+ ret = set_trace_switch(vec[0]);
+ if (ret)
+ return ret;
+ }
send_ack(conn, XS_CONTROL);
return 0;
@@ -923,7 +948,7 @@ static int do_control_help(const void *, struct connection *, char **, int);
static struct cmd_s cmds[] = {
{ "check", do_control_check, "" },
- { "log", do_control_log, "on|off" },
+ { "log", do_control_log, "[on|off|+<switch>|-<switch>]" },
#ifndef NO_LIVE_UPDATE
/*
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 48c2793aeb..0d5b5a0d82 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -85,6 +85,7 @@ static int reopen_log_pipe[2];
static int reopen_log_pipe0_pollfd_idx = -1;
char *tracefile = NULL;
TDB_CONTEXT *tdb_ctx = NULL;
+unsigned int trace_flags = TRACE_OBJ | TRACE_IO;
static const char *sockmsg_string(enum xsd_sockmsg_type type);
@@ -139,13 +140,13 @@ static void trace_io(const struct connection *conn,
time_t now;
struct tm *tm;
- if (tracefd < 0)
+ if (tracefd < 0 || !(trace_flags & TRACE_IO))
return;
now = time(NULL);
tm = localtime(&now);
- trace("%s %p %04d%02d%02d %02d:%02d:%02d %s (",
+ trace("io: %s %p %04d%02d%02d %02d:%02d:%02d %s (",
out ? "OUT" : "IN", conn,
tm->tm_year + 1900, tm->tm_mon + 1,
tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec,
@@ -158,12 +159,14 @@ static void trace_io(const struct connection *conn,
void trace_create(const void *data, const char *type)
{
- trace("CREATE %s %p\n", type, data);
+ if (trace_flags & TRACE_OBJ)
+ trace("obj: CREATE %s %p\n", type, data);
}
void trace_destroy(const void *data, const char *type)
{
- trace("DESTROY %s %p\n", type, data);
+ if (trace_flags & TRACE_OBJ)
+ trace("obj: DESTROY %s %p\n", type, data);
}
/**
@@ -2599,6 +2602,8 @@ static void usage(void)
" -N, --no-fork to request that the daemon does not fork,\n"
" -P, --output-pid to request that the pid of the daemon is output,\n"
" -T, --trace-file <file> giving the file for logging, and\n"
+" --trace-control=+<switch> activate a specific <switch>\n"
+" --trace-control=-<switch> deactivate a specific <switch>\n"
" -E, --entry-nb <nb> limit the number of entries per domain,\n"
" -S, --entry-size <size> limit the size of entry per domain, and\n"
" -W, --watch-nb <nb> limit the number of watches per domain,\n"
@@ -2642,6 +2647,7 @@ static struct option options[] = {
{ "output-pid", 0, NULL, 'P' },
{ "entry-size", 1, NULL, 'S' },
{ "trace-file", 1, NULL, 'T' },
+ { "trace-control", 1, NULL, 1 },
{ "transaction", 1, NULL, 't' },
{ "perm-nb", 1, NULL, 'A' },
{ "path-max", 1, NULL, 'M' },
@@ -2716,6 +2722,43 @@ static void set_quota(const char *arg, bool soft)
barf("unknown quota \"%s\"\n", arg);
}
+/* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
+const char *trace_switches[] = {
+ "obj", "io", "wrl",
+ NULL
+};
+
+int set_trace_switch(const char *arg)
+{
+ bool remove = (arg[0] == '-');
+ unsigned int idx;
+
+ switch (arg[0]) {
+ case '-':
+ remove = true;
+ break;
+ case '+':
+ remove = false;
+ break;
+ default:
+ return EINVAL;
+ }
+
+ arg++;
+
+ for (idx = 0; trace_switches[idx]; idx++) {
+ if (!strcmp(arg, trace_switches[idx])) {
+ if (remove)
+ trace_flags &= ~(1u << idx);
+ else
+ trace_flags |= 1u << idx;
+ return 0;
+ }
+ }
+
+ return EINVAL;
+}
+
int main(int argc, char *argv[])
{
int opt;
@@ -2764,6 +2807,10 @@ int main(int argc, char *argv[])
case 'T':
tracefile = optarg;
break;
+ case 1:
+ if (set_trace_switch(optarg))
+ barf("Illegal trace switch \"%s\"\n", optarg);
+ break;
case 'I':
if (optarg && !strcmp(optarg, "off"))
tdb_flags = 0;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3b96ecd018..c85b15515c 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -287,6 +287,12 @@ extern char **orig_argv;
extern char *tracefile;
extern int tracefd;
+extern unsigned int trace_flags;
+#define TRACE_OBJ 0x00000001
+#define TRACE_IO 0x00000002
+#define TRACE_WRL 0x00000004
+extern const char *trace_switches[];
+int set_trace_switch(const char *arg);
extern TDB_CONTEXT *tdb_ctx;
extern int dom0_domid;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 4d735a5951..28c58b5c6d 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1249,6 +1249,12 @@ static long wrl_ndomains;
static wrl_creditt wrl_reserve; /* [-wrl_config_newdoms_dburst, +_gburst ] */
static time_t wrl_log_last_warning; /* 0: no previous warning */
+#define trace_wrl(...) \
+do { \
+ if (trace_flags & TRACE_WRL) \
+ trace("wrl: " __VA_ARGS__); \
+} while (0)
+
void wrl_gettime_now(struct wrl_timestampt *now_wt)
{
struct timespec now_ts;
@@ -1354,12 +1360,9 @@ void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
domain->wrl_timestamp = now;
- trace("wrl: dom %4d %6ld msec %9ld credit %9ld reserve"
- " %9ld discard\n",
- domain->domid,
- msec,
- (long)domain->wrl_credit, (long)wrl_reserve,
- (long)surplus);
+ trace_wrl("dom %4d %6ld msec %9ld credit %9ld reserve %9ld discard\n",
+ domain->domid, msec, (long)domain->wrl_credit,
+ (long)wrl_reserve, (long)surplus);
}
void wrl_check_timeout(struct domain *domain,
@@ -1391,10 +1394,9 @@ void wrl_check_timeout(struct domain *domain,
if (*ptimeout==-1 || wakeup < *ptimeout)
*ptimeout = wakeup;
- trace("wrl: domain %u credit=%ld (reserve=%ld) SLEEPING for %d\n",
- domain->domid,
- (long)domain->wrl_credit, (long)wrl_reserve,
- wakeup);
+ trace_wrl("domain %u credit=%ld (reserve=%ld) SLEEPING for %d\n",
+ domain->domid, (long)domain->wrl_credit, (long)wrl_reserve,
+ wakeup);
}
#define WRL_LOG(now, ...) \
@@ -1412,9 +1414,8 @@ void wrl_apply_debit_actual(struct domain *domain)
wrl_credit_update(domain, now);
domain->wrl_credit -= wrl_config_writecost;
- trace("wrl: domain %u credit=%ld (reserve=%ld)\n",
- domain->domid,
- (long)domain->wrl_credit, (long)wrl_reserve);
+ trace_wrl("domain %u credit=%ld (reserve=%ld)\n", domain->domid,
+ (long)domain->wrl_credit, (long)wrl_reserve);
if (domain->wrl_credit < 0) {
if (!domain->wrl_delay_logged) {
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 82e5e66c18..1aa9d3cb3d 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -475,6 +475,7 @@ int do_transaction_start(const void *ctx, struct connection *conn,
if (!trans)
return ENOMEM;
+ trace_create(trans, "transaction");
INIT_LIST_HEAD(&trans->accessed);
INIT_LIST_HEAD(&trans->changed_domains);
trans->conn = conn;
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 18/19] tools/xenstore: let check_store() check the accounting data
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (16 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 17/19] tools/xenstore: introduce trace classes Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 19/19] tools/xenstore: make output of "xenstore-control help" more pretty Juergen Gross
2022-12-14 11:06 ` [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Jan Beulich
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Today check_store() is only testing the correctness of the node tree.
Add verification of the accounting data (number of nodes) and correct
the data if it is wrong.
Do the initial check_store() call only after Xenstore entries of a
live update have been read.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstore/xenstored_core.c | 62 ++++++++++++++++------
tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
tools/xenstore/xenstored_domain.h | 4 ++
3 files changed, 137 insertions(+), 15 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 0d5b5a0d82..8fac7c9477 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2384,8 +2384,6 @@ void setup_structure(bool live_update)
manual_node("@introduceDomain", NULL);
domain_nbentry_fix(dom0_domid, 5, true);
}
-
- check_store();
}
static unsigned int hash_from_key_fn(void *k)
@@ -2428,20 +2426,28 @@ int remember_string(struct hashtable *hash, const char *str)
* As we go, we record each node in the given reachable hashtable. These
* entries will be used later in clean_store.
*/
+
+struct check_store_data {
+ struct hashtable *reachable;
+ struct hashtable *domains;
+};
+
static int check_store_step(const void *ctx, struct connection *conn,
struct node *node, void *arg)
{
- struct hashtable *reachable = arg;
+ struct check_store_data *data = arg;
- if (hashtable_search(reachable, (void *)node->name)) {
+ if (hashtable_search(data->reachable, (void *)node->name)) {
log("check_store: '%s' is duplicated!", node->name);
return recovery ? WALK_TREE_RM_CHILDENTRY
: WALK_TREE_SKIP_CHILDREN;
}
- if (!remember_string(reachable, node->name))
+ if (!remember_string(data->reachable, node->name))
return WALK_TREE_ERROR_STOP;
+ domain_check_acc_add(node, data->domains);
+
return WALK_TREE_OK;
}
@@ -2491,37 +2497,61 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val,
* Given the list of reachable nodes, iterate over the whole store, and
* remove any that were not reached.
*/
-static void clean_store(struct hashtable *reachable)
+static void clean_store(struct check_store_data *data)
{
- tdb_traverse(tdb_ctx, &clean_store_, reachable);
+ tdb_traverse(tdb_ctx, &clean_store_, data->reachable);
+ domain_check_acc(data->domains);
}
+int check_store_path(const char *name, struct check_store_data *data)
+{
+ struct node *node;
+
+ node = read_node(NULL, NULL, name);
+ if (!node) {
+ log("check_store: error %d reading special node '%s'", errno,
+ name);
+ return errno;
+ }
+
+ return check_store_step(NULL, NULL, node, data);
+}
void check_store(void)
{
- struct hashtable *reachable;
struct walk_funcs walkfuncs = {
.enter = check_store_step,
.enoent = check_store_enoent,
};
+ struct check_store_data data;
/* Don't free values (they are all void *1) */
- reachable = create_hashtable(NULL, 16, hash_from_key_fn, keys_equal_fn,
- HASHTABLE_FREE_KEY);
- if (!reachable) {
+ data.reachable = create_hashtable(NULL, 16, hash_from_key_fn,
+ keys_equal_fn, HASHTABLE_FREE_KEY);
+ if (!data.reachable) {
log("check_store: ENOMEM");
return;
}
+ data.domains = domain_check_acc_init();
+ if (!data.domains) {
+ log("check_store: ENOMEM");
+ goto out_hash;
+ }
+
log("Checking store ...");
- if (walk_node_tree(NULL, NULL, "/", &walkfuncs, reachable)) {
+ if (walk_node_tree(NULL, NULL, "/", &walkfuncs, &data)) {
if (errno == ENOMEM)
log("check_store: ENOMEM");
- } else if (!check_transactions(reachable))
- clean_store(reachable);
+ } else if (!check_store_path("@introduceDomain", &data) &&
+ !check_store_path("@releaseDomain", &data) &&
+ !check_transactions(data.reachable))
+ clean_store(&data);
log("Checking store complete.");
- hashtable_destroy(reachable);
+ hashtable_destroy(data.domains);
+ out_hash:
+ hashtable_destroy(data.reachable);
}
@@ -2920,6 +2950,8 @@ int main(int argc, char *argv[])
lu_read_state();
#endif
+ check_store();
+
/* Get ready to listen to the tools. */
initialize_fds(&sock_pollfd_idx, &timeout);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 28c58b5c6d..9162cabe48 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1562,6 +1562,92 @@ void read_state_connection(const void *ctx, const void *state)
read_state_buffered_data(ctx, conn, sc);
}
+struct domain_acc {
+ unsigned int domid;
+ int nodes;
+};
+
+static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
+{
+ struct hashtable *domains = arg;
+ struct domain *d = v;
+ struct domain_acc *dom;
+
+ dom = talloc_zero(NULL, struct domain_acc);
+ if (!dom)
+ return -1;
+
+ dom->domid = d->domid;
+ /*
+ * Set the initial value to the negative one of the current domain.
+ * If everything is correct incrementing the value for each node will
+ * result in dom->nodes being 0 at the end.
+ */
+ dom->nodes = -d->nbentry;
+
+ if (!hashtable_insert(domains, &dom->domid, dom)) {
+ talloc_free(dom);
+ return -1;
+ }
+
+ return 0;
+}
+
+struct hashtable *domain_check_acc_init(void)
+{
+ struct hashtable *domains;
+
+ domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn,
+ HASHTABLE_FREE_VALUE);
+ if (!domains)
+ return NULL;
+
+ if (hashtable_iterate(domhash, domain_check_acc_init_sub, domains)) {
+ hashtable_destroy(domains);
+ return NULL;
+ }
+
+ return domains;
+}
+
+void domain_check_acc_add(const struct node *node, struct hashtable *domains)
+{
+ struct domain_acc *dom;
+ unsigned int domid;
+
+ domid = node->perms.p[0].id;
+ dom = hashtable_search(domains, &domid);
+ if (!dom)
+ log("Node %s owned by unknown domain %u", node->name, domid);
+ else
+ dom->nodes++;
+}
+
+static int domain_check_acc_sub(const void *k, void *v, void *arg)
+{
+ struct domain_acc *dom = v;
+ struct domain *d;
+
+ if (!dom->nodes)
+ return 0;
+
+ log("Correct accounting data for domain %u: nodes are %d off",
+ dom->domid, dom->nodes);
+
+ d = find_domain_struct(dom->domid);
+ if (!d)
+ return 0;
+
+ d->nbentry += dom->nodes;
+
+ return 0;
+}
+
+void domain_check_acc(struct hashtable *domains)
+{
+ hashtable_iterate(domains, domain_check_acc_sub, NULL);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 22996e2576..dc4660861e 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -129,4 +129,8 @@ const char *dump_state_connections(FILE *fp);
void read_state_connection(const void *ctx, const void *state);
+struct hashtable *domain_check_acc_init(void);
+void domain_check_acc_add(const struct node *node, struct hashtable *domains);
+void domain_check_acc(struct hashtable *domains);
+
#endif /* _XENSTORED_DOMAIN_H */
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 19/19] tools/xenstore: make output of "xenstore-control help" more pretty
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (17 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 18/19] tools/xenstore: let check_store() check the accounting data Juergen Gross
@ 2022-12-13 16:00 ` Juergen Gross
2022-12-14 11:06 ` [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Jan Beulich
19 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-13 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
Using a tab for separating the command from the options in the output
of "xenstore-control help" results in a rather ugly list.
Use a fixed size for the command instead.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
tools/xenstore/xenstored_control.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 000b2bb8c7..cbd62556c3 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -996,7 +996,7 @@ static int do_control_help(const void *ctx, struct connection *conn,
if (!resp)
return ENOMEM;
for (cmd = 0; cmd < ARRAY_SIZE(cmds); cmd++) {
- resp = talloc_asprintf_append(resp, "%s\t%s\n",
+ resp = talloc_asprintf_append(resp, "%-15s %s\n",
cmds[cmd].cmd, cmds[cmd].pars);
if (!resp)
return ENOMEM;
--
2.35.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno
2022-12-13 16:00 ` [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno Juergen Gross
@ 2022-12-14 9:54 ` Jan Beulich
2022-12-14 10:08 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2022-12-14 9:54 UTC (permalink / raw)
To: Juergen Gross; +Cc: Wei Liu, Julien Grall, Anthony PERARD, xen-devel
On 13.12.2022 17:00, Juergen Gross wrote:
> Today talloc_free() is not guaranteed to preserve errno, especially in
> case a custom destructor is being used.
>
> Change that by renaming talloc_free() to _talloc_free() in talloc.c and
> adding a wrapper to talloc.c.
This looks to be stale ...
> This allows to remove some errno saving outside of talloc.c.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - drop wrapper (Julien Grall)
... after this change?
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno
2022-12-14 9:54 ` Jan Beulich
@ 2022-12-14 10:08 ` Juergen Gross
0 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2022-12-14 10:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Julien Grall, Anthony PERARD, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 657 bytes --]
On 14.12.22 10:54, Jan Beulich wrote:
> On 13.12.2022 17:00, Juergen Gross wrote:
>> Today talloc_free() is not guaranteed to preserve errno, especially in
>> case a custom destructor is being used.
>>
>> Change that by renaming talloc_free() to _talloc_free() in talloc.c and
>> adding a wrapper to talloc.c.
>
> This looks to be stale ...
>
>> This allows to remove some errno saving outside of talloc.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - drop wrapper (Julien Grall)
>
> ... after this change?
Oh yes, indeed.
It should be replaced by:
"So preserve errno in talloc_free()."
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
` (18 preceding siblings ...)
2022-12-13 16:00 ` [PATCH v2 19/19] tools/xenstore: make output of "xenstore-control help" more pretty Juergen Gross
@ 2022-12-14 11:06 ` Jan Beulich
19 siblings, 0 replies; 44+ messages in thread
From: Jan Beulich @ 2022-12-14 11:06 UTC (permalink / raw)
To: Juergen Gross
Cc: Wei Liu, Julien Grall, Anthony PERARD, Andrew Cooper,
George Dunlap, Stefano Stabellini, xen-devel
On 13.12.2022 17:00, Juergen Gross wrote:
> This is a first run of post-XSA patches which piled up during the
> development phase of all the recent Xenstore related XSA patches.
>
> At least the first 5 patches are completely independent from each
> other. After those the dependencies are starting to be more complex.
I've applied the three ones of these which were fully ready.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped
2022-12-13 16:00 ` [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped Juergen Gross
@ 2022-12-20 19:01 ` Julien Grall
2023-01-11 6:36 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 19:01 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> When a domain has been released by Xen tools, remove all its
> registered watches. This avoids sending watch events to the dead domain
> when all the nodes related to it are being removed by the Xen tools.
AFAICT, the only user of the command in the tree is softreset. Would you
be able to check this is still working as expected?
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - move call to do_release() (Julien Grall)
> ---
> tools/xenstore/xenstored_domain.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index aa86892fed..e669c89e94 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -740,6 +740,9 @@ int do_release(const void *ctx, struct connection *conn,
> if (IS_ERR(domain))
> return -PTR_ERR(domain);
>
> + /* Avoid triggering watch events when the domain's nodes are deleted. */
> + conn_delete_all_watches(domain->conn);
> +
> talloc_free(domain->conn);
>
> send_ack(conn, XS_RELEASE);
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid
2022-12-13 16:00 ` [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid Juergen Gross
@ 2022-12-20 19:09 ` Julien Grall
0 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2022-12-20 19:09 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> Today finding a struct domain by its domain id requires to scan the
> list of domains until finding the correct domid.
>
> Add a hashlist for being able to speed this up. This allows to remove
> the linking of struct domain in a list. Note that the list of changed
> domains per transaction is kept as a list, as there are no known use
> cases with more than 4 domains being touched in a single transaction
> (this would be a device handled by a driver domain and being assigned
> to a HVM domain with device model in a stubdom, plus the control
> domain).
>
> Some simple performance tests comparing the scanning and hashlist have
> shown that the hashlist will win as soon as more than 6 entries need
> to be scanned.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
2022-12-13 16:00 ` [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths Juergen Gross
@ 2022-12-20 19:39 ` Julien Grall
2023-01-11 6:41 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 19:39 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> Instead of special casing the permission handling and watch event
> firing for the special watch paths "@introduceDomain" and
> "@releaseDomain", use static dummy nodes added to the data base when
> starting Xenstore.
>
> The node accounting needs to reflect that change by adding the special
> nodes in the domain_entry_fix() call in setup_structure().
>
> Note that this requires to rework the calls of fire_watches() for the
> special events in order to avoid leaking memory.
>
> Move the check for a valid node name from get_node() to
> get_node_canonicalized(), as it allows to use get_node() for the
> special nodes, too.
>
> In order to avoid read and write accesses to the special nodes use a
> special variant for obtaining the current node data for the permission
> handling.
>
> This allows to simplify quite some code. In future sub-nodes of the
> special nodes will be possible due to this change, allowing more fine
> grained permission control of special events for specific domains.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - add get_spec_node()
> - expand commit message (Julien Grall)
> ---
> tools/xenstore/xenstored_control.c | 3 -
> tools/xenstore/xenstored_core.c | 92 +++++++++-------
> tools/xenstore/xenstored_domain.c | 162 ++++-------------------------
> tools/xenstore/xenstored_domain.h | 6 --
> tools/xenstore/xenstored_watch.c | 17 +--
> 5 files changed, 80 insertions(+), 200 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
> index d1aaa00bf4..41e6992591 100644
> --- a/tools/xenstore/xenstored_control.c
> +++ b/tools/xenstore/xenstored_control.c
> @@ -676,9 +676,6 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
> if (ret)
> goto out;
> ret = dump_state_connections(fp);
> - if (ret)
> - goto out;
> - ret = dump_state_special_nodes(fp);
> if (ret)
> goto out;
> ret = dump_state_nodes(fp, ctx);
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 1650821922..f96714e1b8 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -616,7 +616,8 @@ static void get_acc_data(TDB_DATA *key, struct node_account_data *acc)
> static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key,
> unsigned int domid)
> {
> - return (!conn || key->dptr[0] == '/') ? domid : conn->id;
> + return (!conn || key->dptr[0] == '/' || key->dptr[0] == '@')
The comment on top of get_acc_domid() needs to be updated.
> + ? domid : conn->id;
> }
>
[...]
> +static void fire_special_watches(const char *name)
> +{
> + void *ctx = talloc_new(NULL);
> + struct node *node;
> +
> + if (!ctx)
> + return;
> +
> + node = read_node(NULL, ctx, name);
> +
> + if (node)
> + fire_watches(NULL, ctx, name, node, true, NULL);
NIT: I would consider to log an error (maybe only once) if 'node' is
NULL. The purpose is to help debugging Xenstored.
The rest of the code LGTM.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length
2022-12-13 16:00 ` [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length Juergen Gross
@ 2022-12-20 19:42 ` Julien Grall
2023-01-11 6:48 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 19:42 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> @@ -313,19 +302,19 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
> unsigned int conn_id)
> {
> const char *ret = NULL;
> + const char *watch_path;
> struct watch *watch;
> struct xs_state_watch sw;
> struct xs_state_record_header head;
> - const char *path;
>
> head.type = XS_STATE_TYPE_WATCH;
>
> list_for_each_entry(watch, &conn->watches, list) {
> head.length = sizeof(sw);
>
> + watch_path = get_watch_path(watch, watch->node);
It is not clear to me why you call get_watch_path() earlier and also
rename the variable.
I don't mind the new name, but it doesn't feel like it belongs to this
patch as the code in duymp_state_watches() would not be changed otherwise.
Cheers,
> sw.conn_id = conn_id;
> - path = get_watch_path(watch, watch->node);
> - sw.path_length = strlen(path) + 1;
> + sw.path_length = strlen(watch_path) + 1;
> sw.token_length = strlen(watch->token) + 1;
> head.length += sw.path_length + sw.token_length;
> head.length = ROUNDUP(head.length, 3);
> @@ -334,7 +323,7 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
> if (fwrite(&sw, sizeof(sw), 1, fp) != 1)
> return "Dump watch state error";
>
> - if (fwrite(path, sw.path_length, 1, fp) != 1)
> + if (fwrite(watch_path, sw.path_length, 1, fp) != 1)
> return "Dump watch path error";
> if (fwrite(watch->token, sw.token_length, 1, fp) != 1)
> return "Dump watch token error";
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 09/19] tools/xenstore: move changed domain handling
2022-12-13 16:00 ` [PATCH v2 09/19] tools/xenstore: move changed domain handling Juergen Gross
@ 2022-12-20 19:49 ` Julien Grall
0 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2022-12-20 19:49 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> Move all code related to struct changed_domain from
> xenstored_transaction.c to xenstored_domain.c.
>
> This will be needed later in order to simplify the accounting data
> updates in cases of errors during a request.
>
> Split the code to have a more generic base framework.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2022-12-13 16:00 ` [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface Juergen Gross
@ 2022-12-20 20:15 ` Julien Grall
2023-01-11 8:59 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 20:15 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi,
On 13/12/2022 16:00, Juergen Gross wrote:
> Rework the interface and the internals of the per-domain node
> accounting:
>
> - rename the functions to domain_nbentry_*() in order to better match
> the related counter name
>
> - switch from node pointer to domid as interface, as all nodes have the
> owner filled in
The downside is now you have may place open-coding
"...->perms->p[0].id". IHMO this is making the code more complicated. So
can you introduce a few wrappers that would take a node and then convert
to the owner?
>
> - use a common internal function for adding a value to the counter
>
> For the transaction case add a helper function to get the list head
> of the per-transaction changed domains, enabling to eliminate the
> transaction_entry_*() functions.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> tools/xenstore/xenstored_core.c | 22 ++---
> tools/xenstore/xenstored_domain.c | 122 +++++++++++--------------
> tools/xenstore/xenstored_domain.h | 10 +-
> tools/xenstore/xenstored_transaction.c | 15 +--
> tools/xenstore/xenstored_transaction.h | 7 +-
> 5 files changed, 72 insertions(+), 104 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index f96714e1b8..61569cecbb 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, struct node *node)
> static int destroy_node(struct connection *conn, struct node *node)
> {
> destroy_node_rm(conn, node);
> - domain_entry_dec(conn, node);
> + domain_nbentry_dec(conn, node->perms.p[0].id);
>
> /*
> * It is not possible to easily revert the changes in a transaction.
> @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
> for (i = node; i; i = i->parent) {
> /* i->parent is set for each new node, so check quota. */
> if (i->parent &&
> - domain_entry(conn) >= quota_nb_entry_per_domain) {
> + domain_nbentry(conn) >= quota_nb_entry_per_domain) {
> ret = ENOSPC;
> goto err;
> }
> @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
>
> /* Account for new node */
> if (i->parent) {
> - if (domain_entry_inc(conn, i)) {
> + if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
> destroy_node_rm(conn, i);
> return NULL;
> }
> @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection *conn,
> watch_exact = strcmp(root, node->name);
> fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
>
> - domain_entry_dec(conn, node);
> + domain_nbentry_dec(conn, node->perms.p[0].id);
>
> return WALK_TREE_RM_CHILDENTRY;
> }
> @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct connection *conn,
> return EPERM;
>
> old_perms = node->perms;
> - domain_entry_dec(conn, node);
> + domain_nbentry_dec(conn, node->perms.p[0].id);
> node->perms = perms;
> - if (domain_entry_inc(conn, node)) {
> + if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
> node->perms = old_perms;
> /*
> * This should never fail because we had a reference on the
> * domain before and Xenstored is single-threaded.
> */
> - domain_entry_inc(conn, node);
> + domain_nbentry_inc(conn, node->perms.p[0].id);
> return ENOMEM;
> }
>
> if (write_node(conn, node, false)) {
> int saved_errno = errno;
>
> - domain_entry_dec(conn, node);
> + domain_nbentry_dec(conn, node->perms.p[0].id);
> node->perms = old_perms;
> /* No failure possible as above. */
> - domain_entry_inc(conn, node);
> + domain_nbentry_inc(conn, node->perms.p[0].id);
>
> errno = saved_errno;
> return errno;
> @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
> manual_node("/tool/xenstored", NULL);
> manual_node("@releaseDomain", NULL);
> manual_node("@introduceDomain", NULL);
> - domain_entry_fix(dom0_domid, 5, true);
> + domain_nbentry_fix(dom0_domid, 5, true);
> }
>
> check_store();
> @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
> if (write_node_raw(NULL, &key, node, true))
> barf("write node error restoring node");
>
> - if (domain_entry_inc(&conn, node))
> + if (domain_nbentry_inc(&conn, node->perms.p[0].id))
> barf("node accounting error restoring node");
>
> talloc_free(node);
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 3216119e83..40b24056c5 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
> domain->nbentry--;
> node->perms.p[0].id = priv_domid;
> node->acc.memory = 0;
> - domain_entry_inc(NULL, node);
> + domain_nbentry_inc(NULL, priv_domid);
> if (write_node_raw(NULL, &key, node, true)) {
> /* That's unfortunate. We only can try to continue. */
> syslog(LOG_ERR,
> @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update)
> int cnt;
>
> list_for_each_entry(cd, head, list) {
> - cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
> + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
> if (!update) {
> if (cnt >= quota_nb_entry_per_domain)
> return ENOSPC;
> @@ -604,18 +604,19 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx,
> return cd;
> }
>
> -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
> - unsigned int domid)
> +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
> + unsigned int domid)
> {
> struct changed_domain *cd;
>
> cd = acc_get_changed_domain(ctx, head, domid);
> if (!cd)
> - return errno;
> + return 0;
>
> + errno = 0;
> cd->nbentry += val;
>
> - return 0;
> + return cd->nbentry;
You just introduced this helper in the previous patch (i.e. #9). So can
you get the interface correct from the start? This will make easier to
review the series.
I don't mind too much if you add the static here. Although, it would
have been nice if we avoid changing code just introduced.
> }
>
> static void domain_conn_reset(struct domain *domain)
> @@ -988,30 +989,6 @@ void domain_deinit(void)
> xenevtchn_unbind(xce_handle, virq_port);
> }
>
> -int domain_entry_inc(struct connection *conn, struct node *node)
> -{
> - struct domain *d;
> - unsigned int domid;
> -
> - if (!node->perms.p)
> - return 0;
> -
> - domid = node->perms.p[0].id;
> -
> - if (conn && conn->transaction) {
> - transaction_entry_inc(conn->transaction, domid);
> - } else {
> - d = (conn && domid == conn->id && conn->domain) ? conn->domain
> - : find_or_alloc_existing_domain(domid);
> - if (d)
> - d->nbentry++;
> - else
> - return ENOMEM;
> - }
> -
> - return 0;
> -}
> -
> /*
> * Check whether a domain was created before or after a specific generation
> * count (used for testing whether a node permission is older than a domain).
> @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node)
> return 0;
> }
>
> -void domain_entry_dec(struct connection *conn, struct node *node)
> +static int domain_nbentry_add(struct connection *conn, unsigned int domid,
> + int add, bool dom_exists)
The name of the variable suggests that that if it is false then it
doesn't exists. However, looking at how you use it, it is more a "Can
struct domain be allocated?". So I would rename it to
"dom_alloc_allowed" or similar.
> {
> struct domain *d;
> - unsigned int domid;
> -
> - if (!node->perms.p)
> - return;
> + struct list_head *head;
> + int ret;
>
> - domid = node->perms.p ? node->perms.p[0].id : conn->id;
> + if (conn && domid == conn->id && conn->domain)
> + d = conn->domain;
> + else if (dom_exists) {
> + d = find_domain_struct(domid);
> + if (!d) {
> + errno = ENOENT;
> + corrupt(conn, "Missing domain %u\n", domid);
> + return -1;
> + }
> + } else {
> + d = find_or_alloc_existing_domain(domid);
> + if (!d) {
> + errno = ENOMEM;
> + return -1;
> + }
> + }
>
> if (conn && conn->transaction) {
> - transaction_entry_dec(conn->transaction, domid);
> - } else {
> - d = (conn && domid == conn->id && conn->domain) ? conn->domain
> - : find_domain_struct(domid);
> - if (d) {
> - d->nbentry--;
> - } else {
> - errno = ENOENT;
> - corrupt(conn,
> - "Node \"%s\" owned by non-existing domain %u\n",
> - node->name, domid);
> + head = transaction_get_changed_domains(conn->transaction);
> + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
> + if (errno) {
> + fail_transaction(conn->transaction);
> + return -1;
> }
> + return d->nbentry + ret;
It is not entirely clear why you are return "d->nbentry + ret" here. If
it is ...
> }
> +
> + d->nbentry += add;
> +
> + return d->nbentry;
> }
>
> -int domain_entry_fix(unsigned int domid, int num, bool update)
> +int domain_nbentry_inc(struct connection *conn, unsigned int domid)
> {
> - struct domain *d;
> - int cnt;
> + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
> +}
>
> - if (update) {
> - d = find_domain_struct(domid);
> - assert(d);
> - } else {
> - /*
> - * We are called first with update == false in order to catch
> - * any error. So do a possible allocation and check for error
> - * only in this case, as in the case of update == true nothing
> - * can go wrong anymore as the allocation already happened.
> - */
> - d = find_or_alloc_existing_domain(domid);
> - if (!d)
> - return -1;
> - }
> +int domain_nbentry_dec(struct connection *conn, unsigned int domid)
> +{
> + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
... to make sure domain_nbentry_add() is not returning a negative value.
Then it would not work.
A good example imagine you have a transaction removing nodes from tree
but not adding any. Then the "ret" would be negative.
Meanwhile the nodes are also removed outside of the transaction. So the
sum of "d->nbentry + ret" would be negative resulting to a failure here.
Such change of behavior should pointed in the commit message. But then I
am not convinced this should be part of this commit which is mainly
reworking an interface (e.g. no functional change is expected).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction
2022-12-13 16:00 ` [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction Juergen Gross
@ 2022-12-20 20:18 ` Julien Grall
2023-01-11 9:07 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 20:18 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi,
On 13/12/2022 16:00, Juergen Gross wrote:
> The accounting for the number of nodes of a domain in an active
> transaction is not working correctly, as it allows to create arbitrary
> number of nodes. The transaction will finally fail due to exceeding
> the number of nodes quota, but before closing the transaction an
> unprivileged guest could cause Xenstore to use a lot of memory.
As per the discussion in v1, the commit message needs to be reworded.
I will look at this patch in more details once I have reached the 2nd
series.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework
2022-12-13 16:00 ` [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework Juergen Gross
@ 2022-12-20 21:50 ` Julien Grall
2023-01-11 9:27 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2022-12-20 21:50 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
> @@ -115,47 +117,32 @@ hashtable_expand(struct hashtable *h)
> if (h->primeindex == (prime_table_length - 1)) return 0;
> newsize = primes[++(h->primeindex)];
>
> - newtable = (struct entry **)calloc(newsize, sizeof(struct entry*));
> - if (NULL != newtable)
> + newtable = talloc_realloc(h, h->table, struct entry *, newsize);
> + if (!newtable)
> {
> - /* This algorithm is not 'stable'. ie. it reverses the list
> - * when it transfers entries between the tables */
> - for (i = 0; i < h->tablelength; i++) {
> - while (NULL != (e = h->table[i])) {
> - h->table[i] = e->next;
> - index = indexFor(newsize,e->h);
> + h->primeindex--;
> + return 0;
> + }
> +
> + h->table = newtable;
> + memset(newtable + h->tablelength, 0,
> + (newsize - h->tablelength) * sizeof(*newtable));
> + for (i = 0; i < h->tablelength; i++) {
I understand this code is taken from the realloc path. However, isn't
this algorithm also not stable? If so, then I think we move the comment
here.
> + for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
> + index = indexFor(newsize,e->h);
Missing space after ",".
> + if (index == i)
> + {
> + pE = &(e->next);
> + }
> + else
> + {
> + *pE = e->next;
> e->next = newtable[index];
> newtable[index] = e;
> }
> }
> - free(h->table);
> - h->table = newtable;
> - }
> - /* Plan B: realloc instead */
> - else
> - {
> - newtable = (struct entry **)
> - realloc(h->table, newsize * sizeof(struct entry *));
> - if (NULL == newtable) { (h->primeindex)--; return 0; }
> - h->table = newtable;
> - memset(newtable + h->tablelength, 0,
> - (newsize - h->tablelength) * sizeof(*newtable));
> - for (i = 0; i < h->tablelength; i++) {
> - for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
> - index = indexFor(newsize,e->h);
> - if (index == i)
> - {
> - pE = &(e->next);
> - }
> - else
> - {
> - *pE = e->next;
> - e->next = newtable[index];
> - newtable[index] = e;
> - }
> - }
> - }
> }
> +
> h->tablelength = newsize;
> h->loadlimit = (unsigned int)
> (((uint64_t)newsize * max_load_factor) / 100);
> @@ -184,7 +171,7 @@ hashtable_insert(struct hashtable *h, void *k, void *v)
> * element may be ok. Next time we insert, we'll try expanding again.*/
> hashtable_expand(h);
> }
> - e = (struct entry *)calloc(1, sizeof(struct entry));
> + e = talloc_zero(h, struct entry);
> if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
> e->h = hash(h,k);
> index = indexFor(h->tablelength,e->h);
> @@ -238,8 +225,8 @@ hashtable_remove(struct hashtable *h, void *k)
> h->entrycount--;
> v = e->v;
> if (h->flags & HASHTABLE_FREE_KEY)
> - free(e->k);
> - free(e);
> + talloc_free(e->k);
> + talloc_free(e);
> return v;
> }
> pE = &(e->next);
> @@ -280,25 +267,20 @@ void
> hashtable_destroy(struct hashtable *h)
> {
> unsigned int i;
> - struct entry *e, *f;
> + struct entry *e;
> struct entry **table = h->table;
>
> for (i = 0; i < h->tablelength; i++)
> {
> - e = table[i];
> - while (NULL != e)
> + for (e = table[i]; e; e = e->next)
> {
> - f = e;
> - e = e->next;
> if (h->flags & HASHTABLE_FREE_KEY)
> - free(f->k);
> + talloc_free(e->k);
> if (h->flags & HASHTABLE_FREE_VALUE)
> - free(f->v);
> - free(f);
AFAIU, the loop is reworked so you let talloc to free each element with
the parent. Using a while loop is definitely cleaner, but now you will
end up to have two separate loop for the elements.
There is a risk that the overall performance of hashtable_destroy() will
be worse as the data accessed in one loop may not fit in the cache. So
you will have to reload it on the second loop.
Therefore, I think it would be better to keep the loop as-is.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped
2022-12-20 19:01 ` Julien Grall
@ 2023-01-11 6:36 ` Juergen Gross
2023-01-11 17:45 ` Julien Grall
0 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 6:36 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 490 bytes --]
On 20.12.22 20:01, Julien Grall wrote:
> Hi Juergen,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> When a domain has been released by Xen tools, remove all its
>> registered watches. This avoids sending watch events to the dead domain
>> when all the nodes related to it are being removed by the Xen tools.
>
> AFAICT, the only user of the command in the tree is softreset. Would you be able
> to check this is still working as expected?
Seems to work fine.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
2022-12-20 19:39 ` Julien Grall
@ 2023-01-11 6:41 ` Juergen Gross
2023-01-11 17:46 ` Julien Grall
0 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 6:41 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 3617 bytes --]
On 20.12.22 20:39, Julien Grall wrote:
> Hi Juergen,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> Instead of special casing the permission handling and watch event
>> firing for the special watch paths "@introduceDomain" and
>> "@releaseDomain", use static dummy nodes added to the data base when
>> starting Xenstore.
>>
>> The node accounting needs to reflect that change by adding the special
>> nodes in the domain_entry_fix() call in setup_structure().
>>
>> Note that this requires to rework the calls of fire_watches() for the
>> special events in order to avoid leaking memory.
>>
>> Move the check for a valid node name from get_node() to
>> get_node_canonicalized(), as it allows to use get_node() for the
>> special nodes, too.
>>
>> In order to avoid read and write accesses to the special nodes use a
>> special variant for obtaining the current node data for the permission
>> handling.
>>
>> This allows to simplify quite some code. In future sub-nodes of the
>> special nodes will be possible due to this change, allowing more fine
>> grained permission control of special events for specific domains.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - add get_spec_node()
>> - expand commit message (Julien Grall)
>> ---
>> tools/xenstore/xenstored_control.c | 3 -
>> tools/xenstore/xenstored_core.c | 92 +++++++++-------
>> tools/xenstore/xenstored_domain.c | 162 ++++-------------------------
>> tools/xenstore/xenstored_domain.h | 6 --
>> tools/xenstore/xenstored_watch.c | 17 +--
>> 5 files changed, 80 insertions(+), 200 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_control.c
>> b/tools/xenstore/xenstored_control.c
>> index d1aaa00bf4..41e6992591 100644
>> --- a/tools/xenstore/xenstored_control.c
>> +++ b/tools/xenstore/xenstored_control.c
>> @@ -676,9 +676,6 @@ static const char *lu_dump_state(const void *ctx, struct
>> connection *conn)
>> if (ret)
>> goto out;
>> ret = dump_state_connections(fp);
>> - if (ret)
>> - goto out;
>> - ret = dump_state_special_nodes(fp);
>> if (ret)
>> goto out;
>> ret = dump_state_nodes(fp, ctx);
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index 1650821922..f96714e1b8 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -616,7 +616,8 @@ static void get_acc_data(TDB_DATA *key, struct
>> node_account_data *acc)
>> static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key,
>> unsigned int domid)
>> {
>> - return (!conn || key->dptr[0] == '/') ? domid : conn->id;
>> + return (!conn || key->dptr[0] == '/' || key->dptr[0] == '@')
>
> The comment on top of get_acc_domid() needs to be updated.
Oh, yes.
>
>> + ? domid : conn->id;
>> }
>
> [...]
>
>> +static void fire_special_watches(const char *name)
>> +{
>> + void *ctx = talloc_new(NULL);
>> + struct node *node;
>> +
>> + if (!ctx)
>> + return;
>> +
>> + node = read_node(NULL, ctx, name);
>> +
>> + if (node)
>> + fire_watches(NULL, ctx, name, node, true, NULL);
>
> NIT: I would consider to log an error (maybe only once) if 'node' is NULL. The
> purpose is to help debugging Xenstored.
I think we can log it always, as this is really a bad problem.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length
2022-12-20 19:42 ` Julien Grall
@ 2023-01-11 6:48 ` Juergen Gross
0 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 6:48 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 1081 bytes --]
On 20.12.22 20:42, Julien Grall wrote:
> Hi Juergen,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> @@ -313,19 +302,19 @@ const char *dump_state_watches(FILE *fp, struct
>> connection *conn,
>> unsigned int conn_id)
>> {
>> const char *ret = NULL;
>> + const char *watch_path;
>> struct watch *watch;
>> struct xs_state_watch sw;
>> struct xs_state_record_header head;
>> - const char *path;
>> head.type = XS_STATE_TYPE_WATCH;
>> list_for_each_entry(watch, &conn->watches, list) {
>> head.length = sizeof(sw);
>> + watch_path = get_watch_path(watch, watch->node);
>
> It is not clear to me why you call get_watch_path() earlier and also rename the
> variable.
>
> I don't mind the new name, but it doesn't feel like it belongs to this patch as
> the code in duymp_state_watches() would not be changed otherwise.
Both changes are the result of V1 of the series. Will undo them.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2022-12-20 20:15 ` Julien Grall
@ 2023-01-11 8:59 ` Juergen Gross
2023-01-11 17:48 ` Julien Grall
0 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 8:59 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 13667 bytes --]
On 20.12.22 21:15, Julien Grall wrote:
> Hi,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> Rework the interface and the internals of the per-domain node
>> accounting:
>>
>> - rename the functions to domain_nbentry_*() in order to better match
>> the related counter name
>>
>> - switch from node pointer to domid as interface, as all nodes have the
>> owner filled in
>
> The downside is now you have may place open-coding "...->perms->p[0].id". IHMO
> this is making the code more complicated. So can you introduce a few wrappers
> that would take a node and then convert to the owner?
Okay.
>
>>
>> - use a common internal function for adding a value to the counter
>>
>> For the transaction case add a helper function to get the list head
>> of the per-transaction changed domains, enabling to eliminate the
>> transaction_entry_*() functions.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> tools/xenstore/xenstored_core.c | 22 ++---
>> tools/xenstore/xenstored_domain.c | 122 +++++++++++--------------
>> tools/xenstore/xenstored_domain.h | 10 +-
>> tools/xenstore/xenstored_transaction.c | 15 +--
>> tools/xenstore/xenstored_transaction.h | 7 +-
>> 5 files changed, 72 insertions(+), 104 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index f96714e1b8..61569cecbb 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn,
>> struct node *node)
>> static int destroy_node(struct connection *conn, struct node *node)
>> {
>> destroy_node_rm(conn, node);
>> - domain_entry_dec(conn, node);
>> + domain_nbentry_dec(conn, node->perms.p[0].id);
>> /*
>> * It is not possible to easily revert the changes in a transaction.
>> @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn,
>> const void *ctx,
>> for (i = node; i; i = i->parent) {
>> /* i->parent is set for each new node, so check quota. */
>> if (i->parent &&
>> - domain_entry(conn) >= quota_nb_entry_per_domain) {
>> + domain_nbentry(conn) >= quota_nb_entry_per_domain) {
>> ret = ENOSPC;
>> goto err;
>> }
>> @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn,
>> const void *ctx,
>> /* Account for new node */
>> if (i->parent) {
>> - if (domain_entry_inc(conn, i)) {
>> + if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
>> destroy_node_rm(conn, i);
>> return NULL;
>> }
>> @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct
>> connection *conn,
>> watch_exact = strcmp(root, node->name);
>> fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
>> - domain_entry_dec(conn, node);
>> + domain_nbentry_dec(conn, node->perms.p[0].id);
>> return WALK_TREE_RM_CHILDENTRY;
>> }
>> @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct
>> connection *conn,
>> return EPERM;
>> old_perms = node->perms;
>> - domain_entry_dec(conn, node);
>> + domain_nbentry_dec(conn, node->perms.p[0].id);
>> node->perms = perms;
>> - if (domain_entry_inc(conn, node)) {
>> + if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
>> node->perms = old_perms;
>> /*
>> * This should never fail because we had a reference on the
>> * domain before and Xenstored is single-threaded.
>> */
>> - domain_entry_inc(conn, node);
>> + domain_nbentry_inc(conn, node->perms.p[0].id);
>> return ENOMEM;
>> }
>> if (write_node(conn, node, false)) {
>> int saved_errno = errno;
>> - domain_entry_dec(conn, node);
>> + domain_nbentry_dec(conn, node->perms.p[0].id);
>> node->perms = old_perms;
>> /* No failure possible as above. */
>> - domain_entry_inc(conn, node);
>> + domain_nbentry_inc(conn, node->perms.p[0].id);
>> errno = saved_errno;
>> return errno;
>> @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
>> manual_node("/tool/xenstored", NULL);
>> manual_node("@releaseDomain", NULL);
>> manual_node("@introduceDomain", NULL);
>> - domain_entry_fix(dom0_domid, 5, true);
>> + domain_nbentry_fix(dom0_domid, 5, true);
>> }
>> check_store();
>> @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
>> if (write_node_raw(NULL, &key, node, true))
>> barf("write node error restoring node");
>> - if (domain_entry_inc(&conn, node))
>> + if (domain_nbentry_inc(&conn, node->perms.p[0].id))
>> barf("node accounting error restoring node");
>> talloc_free(node);
>> diff --git a/tools/xenstore/xenstored_domain.c
>> b/tools/xenstore/xenstored_domain.c
>> index 3216119e83..40b24056c5 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct
>> connection *conn,
>> domain->nbentry--;
>> node->perms.p[0].id = priv_domid;
>> node->acc.memory = 0;
>> - domain_entry_inc(NULL, node);
>> + domain_nbentry_inc(NULL, priv_domid);
>> if (write_node_raw(NULL, &key, node, true)) {
>> /* That's unfortunate. We only can try to continue. */
>> syslog(LOG_ERR,
>> @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>> int cnt;
>> list_for_each_entry(cd, head, list) {
>> - cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
>> + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>> if (!update) {
>> if (cnt >= quota_nb_entry_per_domain)
>> return ENOSPC;
>> @@ -604,18 +604,19 @@ static struct changed_domain
>> *acc_get_changed_domain(const void *ctx,
>> return cd;
>> }
>> -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
>> - unsigned int domid)
>> +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
>> + unsigned int domid)
>> {
>> struct changed_domain *cd;
>> cd = acc_get_changed_domain(ctx, head, domid);
>> if (!cd)
>> - return errno;
>> + return 0;
>> + errno = 0;
>> cd->nbentry += val;
>> - return 0;
>> + return cd->nbentry;
>
> You just introduced this helper in the previous patch (i.e. #9). So can you get
> the interface correct from the start? This will make easier to review the series.
>
> I don't mind too much if you add the static here. Although, it would have been
> nice if we avoid changing code just introduced.
Fine with me.
>
>> }
>> static void domain_conn_reset(struct domain *domain)
>> @@ -988,30 +989,6 @@ void domain_deinit(void)
>> xenevtchn_unbind(xce_handle, virq_port);
>> }
>> -int domain_entry_inc(struct connection *conn, struct node *node)
>> -{
>> - struct domain *d;
>> - unsigned int domid;
>> -
>> - if (!node->perms.p)
>> - return 0;
>> -
>> - domid = node->perms.p[0].id;
>> -
>> - if (conn && conn->transaction) {
>> - transaction_entry_inc(conn->transaction, domid);
>> - } else {
>> - d = (conn && domid == conn->id && conn->domain) ? conn->domain
>> - : find_or_alloc_existing_domain(domid);
>> - if (d)
>> - d->nbentry++;
>> - else
>> - return ENOMEM;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * Check whether a domain was created before or after a specific generation
>> * count (used for testing whether a node permission is older than a domain).
>> @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node)
>> return 0;
>> }
>> -void domain_entry_dec(struct connection *conn, struct node *node)
>> +static int domain_nbentry_add(struct connection *conn, unsigned int domid,
>> + int add, bool dom_exists)
>
> The name of the variable suggests that that if it is false then it doesn't
> exists. However, looking at how you use it, it is more a "Can struct domain be
> allocated?". So I would rename it to "dom_alloc_allowed" or similar.
I'll name it "no_dom_alloc".
>
>> {
>> struct domain *d;
>> - unsigned int domid;
>> -
>> - if (!node->perms.p)
>> - return;
>> + struct list_head *head;
>> + int ret;
>> - domid = node->perms.p ? node->perms.p[0].id : conn->id;
>> + if (conn && domid == conn->id && conn->domain)
>> + d = conn->domain;
>> + else if (dom_exists) {
>> + d = find_domain_struct(domid);
>> + if (!d) {
>> + errno = ENOENT;
>> + corrupt(conn, "Missing domain %u\n", domid);
>> + return -1;
>> + }
>> + } else {
>> + d = find_or_alloc_existing_domain(domid);
>> + if (!d) {
>> + errno = ENOMEM;
>> + return -1;
>> + }
>> + }
>> if (conn && conn->transaction) {
>> - transaction_entry_dec(conn->transaction, domid);
>> - } else {
>> - d = (conn && domid == conn->id && conn->domain) ? conn->domain
>> - : find_domain_struct(domid);
>> - if (d) {
>> - d->nbentry--;
>> - } else {
>> - errno = ENOENT;
>> - corrupt(conn,
>> - "Node \"%s\" owned by non-existing domain %u\n",
>> - node->name, domid);
>> + head = transaction_get_changed_domains(conn->transaction);
>> + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
>> + if (errno) {
>> + fail_transaction(conn->transaction);
>> + return -1;
>> }
>> + return d->nbentry + ret;
>
> It is not entirely clear why you are return "d->nbentry + ret" here. If it is ...
>
>> }
>> +
>> + d->nbentry += add;
>> +
>> + return d->nbentry;
>> }
>> -int domain_entry_fix(unsigned int domid, int num, bool update)
>> +int domain_nbentry_inc(struct connection *conn, unsigned int domid)
>> {
>> - struct domain *d;
>> - int cnt;
>> + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
>> +}
>> - if (update) {
>> - d = find_domain_struct(domid);
>> - assert(d);
>> - } else {
>> - /*
>> - * We are called first with update == false in order to catch
>> - * any error. So do a possible allocation and check for error
>> - * only in this case, as in the case of update == true nothing
>> - * can go wrong anymore as the allocation already happened.
>> - */
>> - d = find_or_alloc_existing_domain(domid);
>> - if (!d)
>> - return -1;
>> - }
>> +int domain_nbentry_dec(struct connection *conn, unsigned int domid)
>> +{
>> + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
>
> ... to make sure domain_nbentry_add() is not returning a negative value. Then it
> would not work.
>
> A good example imagine you have a transaction removing nodes from tree but not
> adding any. Then the "ret" would be negative.
>
> Meanwhile the nodes are also removed outside of the transaction. So the sum of
> "d->nbentry + ret" would be negative resulting to a failure here.
Thanks for catching this.
I think the correct way to handle this is to return max(d->nbentry + ret, 0) in
domain_nbentry_add(). The value might be imprecise, but always >= 0 and never
wrong outside of a transaction collision.
>
> Such change of behavior should pointed in the commit message. But then I am not
> convinced this should be part of this commit which is mainly reworking an
> interface (e.g. no functional change is expected).
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction
2022-12-20 20:18 ` Julien Grall
@ 2023-01-11 9:07 ` Juergen Gross
0 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 9:07 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 667 bytes --]
On 20.12.22 21:18, Julien Grall wrote:
> Hi,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> The accounting for the number of nodes of a domain in an active
>> transaction is not working correctly, as it allows to create arbitrary
>> number of nodes. The transaction will finally fail due to exceeding
>> the number of nodes quota, but before closing the transaction an
>> unprivileged guest could cause Xenstore to use a lot of memory.
>
> As per the discussion in v1, the commit message needs to be reworded.
>
> I will look at this patch in more details once I have reached the 2nd series.
I'll wait with the rewording until then.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework
2022-12-20 21:50 ` Julien Grall
@ 2023-01-11 9:27 ` Juergen Gross
2023-01-11 17:50 ` Julien Grall
0 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2023-01-11 9:27 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 6268 bytes --]
On 20.12.22 22:50, Julien Grall wrote:
> Hi Juergen,
>
> On 13/12/2022 16:00, Juergen Gross wrote:
>> @@ -115,47 +117,32 @@ hashtable_expand(struct hashtable *h)
>> if (h->primeindex == (prime_table_length - 1)) return 0;
>> newsize = primes[++(h->primeindex)];
>> - newtable = (struct entry **)calloc(newsize, sizeof(struct entry*));
>> - if (NULL != newtable)
>> + newtable = talloc_realloc(h, h->table, struct entry *, newsize);
>> + if (!newtable)
>> {
>> - /* This algorithm is not 'stable'. ie. it reverses the list
>> - * when it transfers entries between the tables */
>> - for (i = 0; i < h->tablelength; i++) {
>> - while (NULL != (e = h->table[i])) {
>> - h->table[i] = e->next;
>> - index = indexFor(newsize,e->h);
>> + h->primeindex--;
>> + return 0;
>> + }
>> +
>> + h->table = newtable;
>> + memset(newtable + h->tablelength, 0,
>> + (newsize - h->tablelength) * sizeof(*newtable));
>> + for (i = 0; i < h->tablelength; i++) {
>
> I understand this code is taken from the realloc path. However, isn't this
> algorithm also not stable? If so, then I think we move the comment here.
I'm fine with that, even if I don't see how it would matter. There is no
guarantee regarding the order of entries for a given index.
>
>> + for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
>> + index = indexFor(newsize,e->h);
>
> Missing space after ",".
Will fix.
>
>> + if (index == i)
>> + {
>> + pE = &(e->next);
>> + }
>> + else
>> + {
>> + *pE = e->next;
>> e->next = newtable[index];
>> newtable[index] = e;
>> }
>> }
>> - free(h->table);
>> - h->table = newtable;
>> - }
>> - /* Plan B: realloc instead */
>> - else
>> - {
>> - newtable = (struct entry **)
>> - realloc(h->table, newsize * sizeof(struct entry *));
>> - if (NULL == newtable) { (h->primeindex)--; return 0; }
>> - h->table = newtable;
>> - memset(newtable + h->tablelength, 0,
>> - (newsize - h->tablelength) * sizeof(*newtable));
>> - for (i = 0; i < h->tablelength; i++) {
>> - for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
>> - index = indexFor(newsize,e->h);
>> - if (index == i)
>> - {
>> - pE = &(e->next);
>> - }
>> - else
>> - {
>> - *pE = e->next;
>> - e->next = newtable[index];
>> - newtable[index] = e;
>> - }
>> - }
>> - }
>> }
>> +
>> h->tablelength = newsize;
>> h->loadlimit = (unsigned int)
>> (((uint64_t)newsize * max_load_factor) / 100);
>> @@ -184,7 +171,7 @@ hashtable_insert(struct hashtable *h, void *k, void *v)
>> * element may be ok. Next time we insert, we'll try expanding again.*/
>> hashtable_expand(h);
>> }
>> - e = (struct entry *)calloc(1, sizeof(struct entry));
>> + e = talloc_zero(h, struct entry);
>> if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
>> e->h = hash(h,k);
>> index = indexFor(h->tablelength,e->h);
>> @@ -238,8 +225,8 @@ hashtable_remove(struct hashtable *h, void *k)
>> h->entrycount--;
>> v = e->v;
>> if (h->flags & HASHTABLE_FREE_KEY)
>> - free(e->k);
>> - free(e);
>> + talloc_free(e->k);
>> + talloc_free(e);
>> return v;
>> }
>> pE = &(e->next);
>> @@ -280,25 +267,20 @@ void
>> hashtable_destroy(struct hashtable *h)
>> {
>> unsigned int i;
>> - struct entry *e, *f;
>> + struct entry *e;
>> struct entry **table = h->table;
>> for (i = 0; i < h->tablelength; i++)
>> {
>> - e = table[i];
>> - while (NULL != e)
>> + for (e = table[i]; e; e = e->next)
>> {
>> - f = e;
>> - e = e->next;
>> if (h->flags & HASHTABLE_FREE_KEY)
>> - free(f->k);
>> + talloc_free(e->k);
>> if (h->flags & HASHTABLE_FREE_VALUE)
>> - free(f->v);
>> - free(f);
>
> AFAIU, the loop is reworked so you let talloc to free each element with the
> parent. Using a while loop is definitely cleaner, but now you will end up to
> have two separate loop for the elements.
>
> There is a risk that the overall performance of hashtable_destroy() will be
> worse as the data accessed in one loop may not fit in the cache. So you will
> have to reload it on the second loop.
>
> Therefore, I think it would be better to keep the loop as-is.
What about a completely different approach? I could make the key and value
talloc children of e when _inserting_ the element and the related flag is
set. This would reduce hashtable_destroy to a single talloc_free().
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped
2023-01-11 6:36 ` Juergen Gross
@ 2023-01-11 17:45 ` Julien Grall
0 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2023-01-11 17:45 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 11/01/2023 06:36, Juergen Gross wrote:
> On 20.12.22 20:01, Julien Grall wrote:
>> On 13/12/2022 16:00, Juergen Gross wrote:
>>> When a domain has been released by Xen tools, remove all its
>>> registered watches. This avoids sending watch events to the dead domain
>>> when all the nodes related to it are being removed by the Xen tools.
>>
>> AFAICT, the only user of the command in the tree is softreset. Would
>> you be able to check this is still working as expected?
>
> Seems to work fine.
Thanks for the confirmation! You can add my reviewed-by:
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
2023-01-11 6:41 ` Juergen Gross
@ 2023-01-11 17:46 ` Julien Grall
0 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2023-01-11 17:46 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 11/01/2023 06:41, Juergen Gross wrote:
> On 20.12.22 20:39, Julien Grall wrote:
>>> +static void fire_special_watches(const char *name)
>>> +{
>>> + void *ctx = talloc_new(NULL);
>>> + struct node *node;
>>> +
>>> + if (!ctx)
>>> + return;
>>> +
>>> + node = read_node(NULL, ctx, name);
>>> +
>>> + if (node)
>>> + fire_watches(NULL, ctx, name, node, true, NULL);
>>
>> NIT: I would consider to log an error (maybe only once) if 'node' is
>> NULL. The purpose is to help debugging Xenstored.
>
> I think we can log it always, as this is really a bad problem.
That works for me. If it is always logged, then I guess this would need
to be a syslog message as well.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2023-01-11 8:59 ` Juergen Gross
@ 2023-01-11 17:48 ` Julien Grall
2023-01-12 5:49 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2023-01-11 17:48 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 11/01/2023 08:59, Juergen Gross wrote:
>> ... to make sure domain_nbentry_add() is not returning a negative
>> value. Then it would not work.
>>
>> A good example imagine you have a transaction removing nodes from tree
>> but not adding any. Then the "ret" would be negative.
>>
>> Meanwhile the nodes are also removed outside of the transaction. So
>> the sum of "d->nbentry + ret" would be negative resulting to a failure
>> here.
>
> Thanks for catching this.
>
> I think the correct way to handle this is to return max(d->nbentry +
> ret, 0) in
> domain_nbentry_add(). The value might be imprecise, but always >= 0 and
> never
> wrong outside of a transaction collision.
I am bit confused with your proposal. If the return value is imprecise,
then what's the point of returning max(...) instead of simply 0?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework
2023-01-11 9:27 ` Juergen Gross
@ 2023-01-11 17:50 ` Julien Grall
0 siblings, 0 replies; 44+ messages in thread
From: Julien Grall @ 2023-01-11 17:50 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi,
On 11/01/2023 09:27, Juergen Gross wrote:
> On 20.12.22 22:50, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 13/12/2022 16:00, Juergen Gross wrote:
>>> @@ -115,47 +117,32 @@ hashtable_expand(struct hashtable *h)
>>> if (h->primeindex == (prime_table_length - 1)) return 0;
>>> newsize = primes[++(h->primeindex)];
>>> - newtable = (struct entry **)calloc(newsize, sizeof(struct entry*));
>>> - if (NULL != newtable)
>>> + newtable = talloc_realloc(h, h->table, struct entry *, newsize);
>>> + if (!newtable)
>>> {
>>> - /* This algorithm is not 'stable'. ie. it reverses the list
>>> - * when it transfers entries between the tables */
>>> - for (i = 0; i < h->tablelength; i++) {
>>> - while (NULL != (e = h->table[i])) {
>>> - h->table[i] = e->next;
>>> - index = indexFor(newsize,e->h);
>>> + h->primeindex--;
>>> + return 0;
>>> + }
>>> +
>>> + h->table = newtable;
>>> + memset(newtable + h->tablelength, 0,
>>> + (newsize - h->tablelength) * sizeof(*newtable));
>>> + for (i = 0; i < h->tablelength; i++) {
>>
>> I understand this code is taken from the realloc path. However, isn't
>> this algorithm also not stable? If so, then I think we move the
>> comment here.
>
> I'm fine with that, even if I don't see how it would matter. There is no
> guarantee regarding the order of entries for a given index.
That's a fair point. Feel free to ignore my comment then :).
>>> + if (index == i)
>>> + {
>>> + pE = &(e->next);
>>> + }
>>> + else
>>> + {
>>> + *pE = e->next;
>>> e->next = newtable[index];
>>> newtable[index] = e;
>>> }
>>> }
>>> - free(h->table);
>>> - h->table = newtable;
>>> - }
>>> - /* Plan B: realloc instead */
>>> - else
>>> - {
>>> - newtable = (struct entry **)
>>> - realloc(h->table, newsize * sizeof(struct entry *));
>>> - if (NULL == newtable) { (h->primeindex)--; return 0; }
>>> - h->table = newtable;
>>> - memset(newtable + h->tablelength, 0,
>>> - (newsize - h->tablelength) * sizeof(*newtable));
>>> - for (i = 0; i < h->tablelength; i++) {
>>> - for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
>>> - index = indexFor(newsize,e->h);
>>> - if (index == i)
>>> - {
>>> - pE = &(e->next);
>>> - }
>>> - else
>>> - {
>>> - *pE = e->next;
>>> - e->next = newtable[index];
>>> - newtable[index] = e;
>>> - }
>>> - }
>>> - }
>>> }
>>> +
>>> h->tablelength = newsize;
>>> h->loadlimit = (unsigned int)
>>> (((uint64_t)newsize * max_load_factor) / 100);
>>> @@ -184,7 +171,7 @@ hashtable_insert(struct hashtable *h, void *k,
>>> void *v)
>>> * element may be ok. Next time we insert, we'll try
>>> expanding again.*/
>>> hashtable_expand(h);
>>> }
>>> - e = (struct entry *)calloc(1, sizeof(struct entry));
>>> + e = talloc_zero(h, struct entry);
>>> if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
>>> e->h = hash(h,k);
>>> index = indexFor(h->tablelength,e->h);
>>> @@ -238,8 +225,8 @@ hashtable_remove(struct hashtable *h, void *k)
>>> h->entrycount--;
>>> v = e->v;
>>> if (h->flags & HASHTABLE_FREE_KEY)
>>> - free(e->k);
>>> - free(e);
>>> + talloc_free(e->k);
>>> + talloc_free(e);
>>> return v;
>>> }
>>> pE = &(e->next);
>>> @@ -280,25 +267,20 @@ void
>>> hashtable_destroy(struct hashtable *h)
>>> {
>>> unsigned int i;
>>> - struct entry *e, *f;
>>> + struct entry *e;
>>> struct entry **table = h->table;
>>> for (i = 0; i < h->tablelength; i++)
>>> {
>>> - e = table[i];
>>> - while (NULL != e)
>>> + for (e = table[i]; e; e = e->next)
>>> {
>>> - f = e;
>>> - e = e->next;
>>> if (h->flags & HASHTABLE_FREE_KEY)
>>> - free(f->k);
>>> + talloc_free(e->k);
>>> if (h->flags & HASHTABLE_FREE_VALUE)
>>> - free(f->v);
>>> - free(f);
>>
>> AFAIU, the loop is reworked so you let talloc to free each element
>> with the parent. Using a while loop is definitely cleaner, but now you
>> will end up to have two separate loop for the elements.
>>
>> There is a risk that the overall performance of hashtable_destroy()
>> will be worse as the data accessed in one loop may not fit in the
>> cache. So you will have to reload it on the second loop.
>>
>> Therefore, I think it would be better to keep the loop as-is.
>
> What about a completely different approach? I could make the key and value
> talloc children of e when _inserting_ the element and the related flag is
> set. This would reduce hashtable_destroy to a single talloc_free().
I am fine with that.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2023-01-11 17:48 ` Julien Grall
@ 2023-01-12 5:49 ` Juergen Gross
2023-01-13 9:53 ` Julien Grall
0 siblings, 1 reply; 44+ messages in thread
From: Juergen Gross @ 2023-01-12 5:49 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 1049 bytes --]
On 11.01.23 18:48, Julien Grall wrote:
> Hi Juergen,
>
> On 11/01/2023 08:59, Juergen Gross wrote:
>>> ... to make sure domain_nbentry_add() is not returning a negative value. Then
>>> it would not work.
>>>
>>> A good example imagine you have a transaction removing nodes from tree but
>>> not adding any. Then the "ret" would be negative.
>>>
>>> Meanwhile the nodes are also removed outside of the transaction. So the sum
>>> of "d->nbentry + ret" would be negative resulting to a failure here.
>>
>> Thanks for catching this.
>>
>> I think the correct way to handle this is to return max(d->nbentry + ret, 0) in
>> domain_nbentry_add(). The value might be imprecise, but always >= 0 and never
>> wrong outside of a transaction collision.
>
> I am bit confused with your proposal. If the return value is imprecise, then
> what's the point of returning max(...) instead of simply 0?
Please have a look at the use case especially in domain_nbentry(). Returning
always 0 would clearly break quota checks.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2023-01-12 5:49 ` Juergen Gross
@ 2023-01-13 9:53 ` Julien Grall
2023-01-13 9:57 ` Juergen Gross
0 siblings, 1 reply; 44+ messages in thread
From: Julien Grall @ 2023-01-13 9:53 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 12/01/2023 05:49, Juergen Gross wrote:
> On 11.01.23 18:48, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 11/01/2023 08:59, Juergen Gross wrote:
>>>> ... to make sure domain_nbentry_add() is not returning a negative
>>>> value. Then it would not work.
>>>>
>>>> A good example imagine you have a transaction removing nodes from
>>>> tree but not adding any. Then the "ret" would be negative.
>>>>
>>>> Meanwhile the nodes are also removed outside of the transaction. So
>>>> the sum of "d->nbentry + ret" would be negative resulting to a
>>>> failure here.
>>>
>>> Thanks for catching this.
>>>
>>> I think the correct way to handle this is to return max(d->nbentry +
>>> ret, 0) in
>>> domain_nbentry_add(). The value might be imprecise, but always >= 0
>>> and never
>>> wrong outside of a transaction collision.
>>
>> I am bit confused with your proposal. If the return value is
>> imprecise, then what's the point of returning max(...) instead of
>> simply 0?
>
> Please have a look at the use case especially in domain_nbentry().
> Returning
> always 0 would clearly break quota checks.
I am a bit concerned that we would have a code checking the quota based
on an imprecise value.
At the moment, I don't have a better suggestion. But we should at least
document in the code when we think the value is imprecise and explain
why bypassing the quota check is OK (IOW who will check it?).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
2023-01-13 9:53 ` Julien Grall
@ 2023-01-13 9:57 ` Juergen Gross
0 siblings, 0 replies; 44+ messages in thread
From: Juergen Gross @ 2023-01-13 9:57 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 1737 bytes --]
On 13.01.23 10:53, Julien Grall wrote:
> Hi Juergen,
>
> On 12/01/2023 05:49, Juergen Gross wrote:
>> On 11.01.23 18:48, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 11/01/2023 08:59, Juergen Gross wrote:
>>>>> ... to make sure domain_nbentry_add() is not returning a negative value.
>>>>> Then it would not work.
>>>>>
>>>>> A good example imagine you have a transaction removing nodes from tree but
>>>>> not adding any. Then the "ret" would be negative.
>>>>>
>>>>> Meanwhile the nodes are also removed outside of the transaction. So the sum
>>>>> of "d->nbentry + ret" would be negative resulting to a failure here.
>>>>
>>>> Thanks for catching this.
>>>>
>>>> I think the correct way to handle this is to return max(d->nbentry + ret, 0) in
>>>> domain_nbentry_add(). The value might be imprecise, but always >= 0 and never
>>>> wrong outside of a transaction collision.
>>>
>>> I am bit confused with your proposal. If the return value is imprecise, then
>>> what's the point of returning max(...) instead of simply 0?
>>
>> Please have a look at the use case especially in domain_nbentry(). Returning
>> always 0 would clearly break quota checks.
>
> I am a bit concerned that we would have a code checking the quota based on an
> imprecise value.
>
> At the moment, I don't have a better suggestion. But we should at least document
> in the code when we think the value is imprecise and explain why bypassing the
> quota check is OK (IOW who will check it?).
The imprecise value will never be too low, it can only be too high (i.e. 0
instead of negative), and that will only happen in a transaction which can't
succeed.
Adding a comment is good idea, though.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2023-01-13 9:58 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 16:00 [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Juergen Gross
2022-12-13 16:00 ` [PATCH v2 01/19] tools/xenstore: let talloc_free() preserve errno Juergen Gross
2022-12-14 9:54 ` Jan Beulich
2022-12-14 10:08 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 02/19] tools/xenstore: let tdb_logger() " Juergen Gross
2022-12-13 16:00 ` [PATCH v2 03/19] tools/xenstore: preserve errno across corrupt() Juergen Gross
2022-12-13 16:00 ` [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped Juergen Gross
2022-12-20 19:01 ` Julien Grall
2023-01-11 6:36 ` Juergen Gross
2023-01-11 17:45 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 05/19] tools/xenstore: enhance hashtable implementation Juergen Gross
2022-12-13 16:00 ` [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid Juergen Gross
2022-12-20 19:09 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths Juergen Gross
2022-12-20 19:39 ` Julien Grall
2023-01-11 6:41 ` Juergen Gross
2023-01-11 17:46 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length Juergen Gross
2022-12-20 19:42 ` Julien Grall
2023-01-11 6:48 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 09/19] tools/xenstore: move changed domain handling Juergen Gross
2022-12-20 19:49 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface Juergen Gross
2022-12-20 20:15 ` Julien Grall
2023-01-11 8:59 ` Juergen Gross
2023-01-11 17:48 ` Julien Grall
2023-01-12 5:49 ` Juergen Gross
2023-01-13 9:53 ` Julien Grall
2023-01-13 9:57 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction Juergen Gross
2022-12-20 20:18 ` Julien Grall
2023-01-11 9:07 ` Juergen Gross
2022-12-13 16:00 ` [PATCH v2 12/19] tools/xenstore: replace literal domid 0 with dom0_domid Juergen Gross
2022-12-13 16:00 ` [PATCH v2 13/19] tools/xenstore: make domain_is_unprivileged() an inline function Juergen Gross
2022-12-13 16:00 ` [PATCH v2 14/19] tools/xenstore: let chk_domain_generation() return a bool Juergen Gross
2022-12-13 16:00 ` [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework Juergen Gross
2022-12-20 21:50 ` Julien Grall
2023-01-11 9:27 ` Juergen Gross
2023-01-11 17:50 ` Julien Grall
2022-12-13 16:00 ` [PATCH v2 16/19] tools/xenstore: make log macro globally available Juergen Gross
2022-12-13 16:00 ` [PATCH v2 17/19] tools/xenstore: introduce trace classes Juergen Gross
2022-12-13 16:00 ` [PATCH v2 18/19] tools/xenstore: let check_store() check the accounting data Juergen Gross
2022-12-13 16:00 ` [PATCH v2 19/19] tools/xenstore: make output of "xenstore-control help" more pretty Juergen Gross
2022-12-14 11:06 ` [PATCH v2 00/19] tools/xenstore: do some cleanup and fixes Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.