[PATCH v3 00/16] tools/xenstore: more cleanups

[PATCH v3 00/16] tools/xenstore: more cleanups

[Juergen Gross]

Some more cleanups of Xenstore.

Based on top of the previous Xenstore series "tools/xenstore: rework
internal accounting".

Changes in V2:
- rebase
- one small modification of patch 10
- added patches 11-13

Changes in V3:
- rebase
- modified patch 4
- added patches 10, 11 and 13

Juergen Gross (16):
  tools/xenstore: verify command line parameters better
  tools/xenstore: do some cleanup of hashtable.c
  tools/xenstore: modify interface of create_hashtable()
  tools/xenstore: rename hashtable_insert() and let it return 0 on
    success
  tools/xenstore: make some write limit functions static
  tools/xenstore: switch write limiting to use millisecond time base
  tools/xenstore: remove stale TODO file
  tools/xenstore: remove unused events list
  tools/xenstore: remove support of file backed data base
  tools/libs/store: use xen_list.h instead of xenstore/list.h
  tools/libs/store: make libxenstore independent of utils.h
  tools/xenstore: remove no longer needed functions from xs_lib.c
  tools/xenstore: replace xs_lib.c with a header
  tools/xenstore: split out environment specific live update code
  tools/xenstore: split out rest of live update control code
  tools/xenstore: remove unused stuff from list.h

 .gitignore                                |   1 -
 MAINTAINERS                               |   1 +
 tools/include/xen-tools/xenstore-common.h | 128 +++++
 tools/include/xenstore.h                  |   3 +
 tools/include/xenstore_lib.h              |   3 -
 tools/libs/store/Makefile                 |   4 -
 tools/libs/store/xs.c                     | 131 +++--
 tools/xenstore/COPYING                    | 514 -----------------
 tools/xenstore/Makefile                   |   7 +-
 tools/xenstore/Makefile.common            |  12 +-
 tools/xenstore/TODO                       |  10 -
 tools/xenstore/hashtable.c                |  98 ++--
 tools/xenstore/hashtable.h                |  22 +-
 tools/xenstore/list.h                     | 227 --------
 tools/xenstore/xenstore_client.c          | 133 ++++-
 tools/xenstore/xenstored_control.c        | 661 +---------------------
 tools/xenstore/xenstored_control.h        |   8 -
 tools/xenstore/xenstored_core.c           |  72 +--
 tools/xenstore/xenstored_core.h           |   7 +-
 tools/xenstore/xenstored_domain.c         | 458 ++++++++-------
 tools/xenstore/xenstored_domain.h         |  24 +-
 tools/xenstore/xenstored_lu.c             | 400 +++++++++++++
 tools/xenstore/xenstored_lu.h             |  81 +++
 tools/xenstore/xenstored_lu_daemon.c      | 133 +++++
 tools/xenstore/xenstored_lu_minios.c      | 121 ++++
 tools/xenstore/xenstored_transaction.c    |   4 +-
 tools/xenstore/xenstored_watch.c          |   5 -
 tools/xenstore/xs_lib.c                   | 292 ----------
 tools/xenstore/xs_lib.h                   |  50 --
 tools/xenstore/xs_tdb_dump.c              |  86 ---
 30 files changed, 1409 insertions(+), 2287 deletions(-)
 create mode 100644 tools/include/xen-tools/xenstore-common.h
 delete mode 100644 tools/xenstore/COPYING
 delete mode 100644 tools/xenstore/TODO
 create mode 100644 tools/xenstore/xenstored_lu.c
 create mode 100644 tools/xenstore/xenstored_lu.h
 create mode 100644 tools/xenstore/xenstored_lu_daemon.c
 create mode 100644 tools/xenstore/xenstored_lu_minios.c
 delete mode 100644 tools/xenstore/xs_lib.c
 delete mode 100644 tools/xenstore/xs_lib.h
 delete mode 100644 tools/xenstore/xs_tdb_dump.c

-- 
2.35.3

[PATCH v3 01/16] tools/xenstore: verify command line parameters better

[Juergen Gross]

Add some more verification of command line parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_core.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 6f03f6687a..22df395aac 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2820,7 +2820,7 @@ int main(int argc, char *argv[])
 			no_domain_init = true;
 			break;
 		case 'E':
-			hard_quotas[ACC_NODES].val = strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_NODES].val = get_optval_uint(optarg);
 			break;
 		case 'F':
 			pidfile = optarg;
@@ -2838,10 +2838,10 @@ int main(int argc, char *argv[])
 			recovery = false;
 			break;
 		case 'S':
-			hard_quotas[ACC_NODESZ].val = strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_NODESZ].val = get_optval_uint(optarg);
 			break;
 		case 't':
-			hard_quotas[ACC_TRANS].val = strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_TRANS].val = get_optval_uint(optarg);
 			break;
 		case 'T':
 			tracefile = optarg;
@@ -2861,14 +2861,13 @@ int main(int argc, char *argv[])
 			verbose = true;
 			break;
 		case 'W':
-			hard_quotas[ACC_WATCH].val = strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_WATCH].val = get_optval_uint(optarg);
 			break;
 		case 'A':
-			hard_quotas[ACC_NPERM].val = strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_NPERM].val = get_optval_uint(optarg);
 			break;
 		case 'M':
-			hard_quotas[ACC_PATHLEN].val =
-				strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_PATHLEN].val = get_optval_uint(optarg);
 			hard_quotas[ACC_PATHLEN].val =
 				 min((unsigned int)XENSTORE_REL_PATH_MAX,
 				     hard_quotas[ACC_PATHLEN].val);
@@ -2883,13 +2882,13 @@ int main(int argc, char *argv[])
 			set_timeout(optarg);
 			break;
 		case 'e':
-			dom0_event = strtol(optarg, NULL, 10);
+			dom0_event = get_optval_uint(optarg);
 			break;
 		case 'm':
-			dom0_domid = strtol(optarg, NULL, 10);
+			dom0_domid = get_optval_uint(optarg);
 			break;
 		case 'p':
-			priv_domid = strtol(optarg, NULL, 10);
+			priv_domid = get_optval_uint(optarg);
 			break;
 #ifndef NO_LIVE_UPDATE
 		case 'U':
-- 
2.35.3

[PATCH v3 02/16] tools/xenstore: do some cleanup of hashtable.c

[Juergen Gross]

Do the following cleanups:
- hashtable_count() isn't used at all, so remove it
- replace prime_table_length and max_load_factor with macros
- make hash() static
- add a loadlimit() helper function
- remove the /***/ lines between functions
- do some style corrections

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
V3:
- more style corrections (Julien Grall)
---
 tools/xenstore/hashtable.c | 76 +++++++++++++++-----------------------
 tools/xenstore/hashtable.h | 10 -----
 2 files changed, 30 insertions(+), 56 deletions(-)

diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index 3d4466b597..dc209158fa 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -40,22 +40,25 @@ static const unsigned int primes[] = {
 50331653, 100663319, 201326611, 402653189,
 805306457, 1610612741
 };
-const unsigned int prime_table_length = sizeof(primes)/sizeof(primes[0]);
-const unsigned int max_load_factor = 65; /* percentage */
 
-/*****************************************************************************/
-/* indexFor */
-static inline unsigned int
-indexFor(unsigned int tablelength, unsigned int hashvalue) {
+#define PRIME_TABLE_LEN   ARRAY_SIZE(primes)
+#define MAX_LOAD_PERCENT  65
+
+static inline unsigned int indexFor(unsigned int tablelength,
+                                    unsigned int hashvalue)
+{
     return (hashvalue % tablelength);
 }
 
-/*****************************************************************************/
-struct hashtable *
-create_hashtable(const void *ctx, unsigned int minsize,
-                 unsigned int (*hashf) (const void *),
-                 int (*eqf) (const void *, const void *),
-                 unsigned int flags)
+static unsigned int loadlimit(unsigned int pindex)
+{
+    return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
+}
+
+struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
+                                   unsigned int (*hashf) (const void *),
+                                   int (*eqf) (const void *, const void *),
+                                   unsigned int flags)
 {
     struct hashtable *h;
     unsigned int pindex, size = primes[0];
@@ -64,8 +67,11 @@ create_hashtable(const void *ctx, unsigned int minsize,
     if (minsize > (1u << 30)) return NULL;
 
     /* Enforce size as prime */
-    for (pindex=0; pindex < prime_table_length; pindex++) {
-        if (primes[pindex] > minsize) { size = primes[pindex]; break; }
+    for (pindex = 0; pindex < PRIME_TABLE_LEN; pindex++) {
+        if (primes[pindex] > minsize) {
+            size = primes[pindex];
+            break;
+        }
     }
 
     h = talloc_zero(ctx, struct hashtable);
@@ -81,7 +87,7 @@ create_hashtable(const void *ctx, unsigned int minsize,
     h->entrycount   = 0;
     h->hashfn       = hashf;
     h->eqfn         = eqf;
-    h->loadlimit    = (unsigned int)(((uint64_t)size * max_load_factor) / 100);
+    h->loadlimit    = loadlimit(pindex);
     return h;
 
 err1:
@@ -90,9 +96,7 @@ err0:
    return NULL;
 }
 
-/*****************************************************************************/
-unsigned int
-hash(const struct hashtable *h, const void *k)
+static unsigned int hash(const struct hashtable *h, const void *k)
 {
     /* Aim to protect against poor hash functions by adding logic here
      * - logic taken from java 1.4 hashtable source */
@@ -104,9 +108,7 @@ hash(const struct hashtable *h, const void *k)
     return i;
 }
 
-/*****************************************************************************/
-static int
-hashtable_expand(struct hashtable *h)
+static int hashtable_expand(struct hashtable *h)
 {
     /* Double the size of the table to accomodate more entries */
     struct entry **newtable;
@@ -114,7 +116,7 @@ hashtable_expand(struct hashtable *h)
     struct entry **pE;
     unsigned int newsize, i, index;
     /* Check we're not hitting max capacity */
-    if (h->primeindex == (prime_table_length - 1)) return 0;
+    if (h->primeindex == (PRIME_TABLE_LEN - 1)) return 0;
     newsize = primes[++(h->primeindex)];
 
     newtable = talloc_realloc(h, h->table, struct entry *, newsize);
@@ -144,21 +146,11 @@ hashtable_expand(struct hashtable *h)
     }
 
     h->tablelength = newsize;
-    h->loadlimit   = (unsigned int)
-        (((uint64_t)newsize * max_load_factor) / 100);
+    h->loadlimit   = loadlimit(h->primeindex);
     return -1;
 }
 
-/*****************************************************************************/
-unsigned int
-hashtable_count(const struct hashtable *h)
-{
-    return h->entrycount;
-}
-
-/*****************************************************************************/
-int
-hashtable_insert(struct hashtable *h, void *k, void *v)
+int hashtable_insert(struct hashtable *h, void *k, void *v)
 {
     /* This method allows duplicate keys - but they shouldn't be used */
     unsigned int index;
@@ -186,9 +178,7 @@ hashtable_insert(struct hashtable *h, void *k, void *v)
     return -1;
 }
 
-/*****************************************************************************/
-void * /* returns value associated with key */
-hashtable_search(const struct hashtable *h, const void *k)
+void *hashtable_search(const struct hashtable *h, const void *k)
 {
     struct entry *e;
     unsigned int hashvalue, index;
@@ -204,7 +194,6 @@ hashtable_search(const struct hashtable *h, const void *k)
     return NULL;
 }
 
-/*****************************************************************************/
 void
 hashtable_remove(struct hashtable *h, const void *k)
 {
@@ -234,10 +223,8 @@ hashtable_remove(struct hashtable *h, const void *k)
     }
 }
 
-/*****************************************************************************/
-int
-hashtable_iterate(struct hashtable *h,
-                  int (*func)(const void *k, void *v, void *arg), void *arg)
+int hashtable_iterate(struct hashtable *h,
+                      int (*func)(const void *k, void *v, void *arg), void *arg)
 {
     int ret;
     unsigned int i;
@@ -260,10 +247,7 @@ hashtable_iterate(struct hashtable *h,
     return 0;
 }
 
-/*****************************************************************************/
-/* destroy */
-void
-hashtable_destroy(struct hashtable *h)
+void hashtable_destroy(struct hashtable *h)
 {
     talloc_free(h);
 }
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index cc0090f133..04310783b6 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -74,16 +74,6 @@ hashtable_search(const struct hashtable *h, const void *k);
 void
 hashtable_remove(struct hashtable *h, const void *k);
 
-/*****************************************************************************
- * hashtable_count
-   
- * @name        hashtable_count
- * @param   h   the hashtable
- * @return      the number of items stored in the hashtable
- */
-unsigned int
-hashtable_count(const struct hashtable *h);
-
 /*****************************************************************************
  * hashtable_iterate
 
-- 
2.35.3

[PATCH v3 03/16] tools/xenstore: modify interface of create_hashtable()

[Juergen Gross]

The minsize parameter of create_hashtable() doesn't have any real use
case for Xenstore, so drop it.

For better talloc_report_full() diagnostic output add a name parameter
to create_hashtable().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- make code more readable (Julien Grall)
---
 tools/xenstore/hashtable.c        | 23 ++++++-----------------
 tools/xenstore/hashtable.h        |  4 ++--
 tools/xenstore/xenstored_core.c   |  2 +-
 tools/xenstore/xenstored_domain.c |  4 ++--
 4 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index dc209158fa..3d2d3a0e22 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -55,39 +55,28 @@ static unsigned int loadlimit(unsigned int pindex)
     return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
 }
 
-struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
+struct hashtable *create_hashtable(const void *ctx, const char *name,
                                    unsigned int (*hashf) (const void *),
                                    int (*eqf) (const void *, const void *),
                                    unsigned int flags)
 {
     struct hashtable *h;
-    unsigned int pindex, size = primes[0];
-
-    /* Check requested hashtable isn't too large */
-    if (minsize > (1u << 30)) return NULL;
-
-    /* Enforce size as prime */
-    for (pindex = 0; pindex < PRIME_TABLE_LEN; pindex++) {
-        if (primes[pindex] > minsize) {
-            size = primes[pindex];
-            break;
-        }
-    }
 
     h = talloc_zero(ctx, struct hashtable);
     if (NULL == h)
         goto err0;
-    h->table = talloc_zero_array(h, struct entry *, size);
+    talloc_set_name_const(h, name);
+    h->table = talloc_zero_array(h, struct entry *, primes[0]);
     if (NULL == h->table)
         goto err1;
 
-    h->tablelength  = size;
+    h->primeindex   = 0;
+    h->tablelength  = primes[h->primeindex];
     h->flags        = flags;
-    h->primeindex   = pindex;
     h->entrycount   = 0;
     h->hashfn       = hashf;
     h->eqfn         = eqf;
-    h->loadlimit    = loadlimit(pindex);
+    h->loadlimit    = loadlimit(h->primeindex);
     return h;
 
 err1:
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 04310783b6..0e1a6f61c2 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -10,7 +10,7 @@ struct hashtable;
    
  * @name                    create_hashtable
  * @param   ctx             talloc context to use for allocations
- * @param   minsize         minimum initial size of hashtable
+ * @param   name            talloc name of the hashtable
  * @param   hashfunction    function for hashing keys
  * @param   key_eq_fn       function for determining key equality
  * @param   flags           flags HASHTABLE_*
@@ -23,7 +23,7 @@ struct hashtable;
 #define HASHTABLE_FREE_KEY   (1U << 1)
 
 struct hashtable *
-create_hashtable(const void *ctx, unsigned int minsize,
+create_hashtable(const void *ctx, const char *name,
                  unsigned int (*hashfunction) (const void *),
                  int (*key_eq_fn) (const void *, const void *),
                  unsigned int flags
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 22df395aac..418790d8d7 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2521,7 +2521,7 @@ void check_store(void)
 	struct check_store_data data;
 
 	/* Don't free values (they are all void *1) */
-	data.reachable = create_hashtable(NULL, 16, hash_from_key_fn,
+	data.reachable = create_hashtable(NULL, "checkstore", hash_from_key_fn,
 					  keys_equal_fn, HASHTABLE_FREE_KEY);
 	if (!data.reachable) {
 		log("check_store: ENOMEM");
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index a641f633a5..a1c91ef3f3 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1015,7 +1015,7 @@ void domain_init(int evtfd)
 	int rc;
 
 	/* Start with a random rather low domain count for the hashtable. */
-	domhash = create_hashtable(NULL, 8, domhash_fn, domeq_fn, 0);
+	domhash = create_hashtable(NULL, "domains", domhash_fn, domeq_fn, 0);
 	if (!domhash)
 		barf_perror("Failed to allocate domain hashtable");
 
@@ -1807,7 +1807,7 @@ struct hashtable *domain_check_acc_init(void)
 {
 	struct hashtable *domains;
 
-	domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn,
+	domains = create_hashtable(NULL, "domain_check", domhash_fn, domeq_fn,
 				   HASHTABLE_FREE_VALUE);
 	if (!domains)
 		return NULL;
-- 
2.35.3

[PATCH v3 04/16] tools/xenstore: rename hashtable_insert() and let it return 0 on success

[Juergen Gross]

Today hashtable_insert() returns 0 in case of an error. Change that to
let it return an errno value in the error case and 0 in case of success.
In order to avoid any missed return value checks or related future
backport errors, rename hashtable_insert() to hashtable_add().

Even if not used today, do the same switch for the return value of
hashtable_expand().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- rename to hashtable_add() (triggered by Julien Grall)
---
 tools/xenstore/hashtable.c             | 17 +++++++++++------
 tools/xenstore/hashtable.h             |  8 ++++----
 tools/xenstore/xenstored_core.c        |  6 +++---
 tools/xenstore/xenstored_domain.c      |  4 ++--
 tools/xenstore/xenstored_transaction.c |  4 ++--
 5 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index 3d2d3a0e22..11f6bf8f15 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -105,14 +105,15 @@ static int hashtable_expand(struct hashtable *h)
     struct entry **pE;
     unsigned int newsize, i, index;
     /* Check we're not hitting max capacity */
-    if (h->primeindex == (PRIME_TABLE_LEN - 1)) return 0;
+    if (h->primeindex == (PRIME_TABLE_LEN - 1))
+        return ENOSPC;
     newsize = primes[++(h->primeindex)];
 
     newtable = talloc_realloc(h, h->table, struct entry *, newsize);
     if (!newtable)
     {
         h->primeindex--;
-        return 0;
+        return ENOMEM;
     }
 
     h->table = newtable;
@@ -136,10 +137,10 @@ static int hashtable_expand(struct hashtable *h)
 
     h->tablelength = newsize;
     h->loadlimit   = loadlimit(h->primeindex);
-    return -1;
+    return 0;
 }
 
-int hashtable_insert(struct hashtable *h, void *k, void *v)
+int hashtable_add(struct hashtable *h, void *k, void *v)
 {
     /* This method allows duplicate keys - but they shouldn't be used */
     unsigned int index;
@@ -153,7 +154,11 @@ int hashtable_insert(struct hashtable *h, void *k, void *v)
         hashtable_expand(h);
     }
     e = talloc_zero(h, struct entry);
-    if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
+    if (NULL == e)
+    {
+        --h->entrycount;
+       return ENOMEM;
+    }
     e->h = hash(h,k);
     index = indexFor(h->tablelength,e->h);
     e->k = k;
@@ -164,7 +169,7 @@ int hashtable_insert(struct hashtable *h, void *k, void *v)
         talloc_steal(e, v);
     e->next = h->table[index];
     h->table[index] = e;
-    return -1;
+    return 0;
 }
 
 void *hashtable_search(const struct hashtable *h, const void *k)
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 0e1a6f61c2..5a2cc4a4be 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -30,13 +30,13 @@ create_hashtable(const void *ctx, const char *name,
 );
 
 /*****************************************************************************
- * hashtable_insert
+ * hashtable_add
    
- * @name        hashtable_insert
+ * @name        hashtable_add
  * @param   h   the hashtable to insert into
  * @param   k   the key - hashtable claims ownership and will free on removal
  * @param   v   the value - does not claim ownership
- * @return      non-zero for successful insertion
+ * @return      zero for successful insertion
  *
  * This function will cause the table to expand if the insertion would take
  * the ratio of entries to table size over the maximum load factor.
@@ -49,7 +49,7 @@ create_hashtable(const void *ctx, const char *name,
  */
 
 int 
-hashtable_insert(struct hashtable *h, void *k, void *v);
+hashtable_add(struct hashtable *h, void *k, void *v);
 
 /*****************************************************************************
  * hashtable_search
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 418790d8d7..c467a704a1 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2404,8 +2404,8 @@ int remember_string(struct hashtable *hash, const char *str)
 	char *k = talloc_strdup(NULL, str);
 
 	if (!k)
-		return 0;
-	return hashtable_insert(hash, k, (void *)1);
+		return ENOMEM;
+	return hashtable_add(hash, k, (void *)1);
 }
 
 /**
@@ -2438,7 +2438,7 @@ static int check_store_step(const void *ctx, struct connection *conn,
 				: WALK_TREE_SKIP_CHILDREN;
 	}
 
-	if (!remember_string(data->reachable, node->name))
+	if (remember_string(data->reachable, node->name))
 		return WALK_TREE_ERROR_STOP;
 
 	domain_check_acc_add(node, data->domains);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index a1c91ef3f3..815f15cd1d 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -538,7 +538,7 @@ static struct domain *alloc_domain(const void *context, unsigned int domid)
 	domain->generation = generation;
 	domain->introduced = false;
 
-	if (!hashtable_insert(domhash, &domain->domid, domain)) {
+	if (hashtable_add(domhash, &domain->domid, domain)) {
 		talloc_free(domain);
 		errno = ENOMEM;
 		return NULL;
@@ -1795,7 +1795,7 @@ static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
 	 */
 	dom->nodes = -d->acc[ACC_NODES].val;
 
-	if (!hashtable_insert(domains, &dom->domid, dom)) {
+	if (hashtable_add(domains, &dom->domid, dom)) {
 		talloc_free(dom);
 		return -1;
 	}
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index db06d0e7f1..334f1609f1 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -601,13 +601,13 @@ int check_transactions(struct hashtable *hash)
 		list_for_each_entry(trans, &conn->transaction_list, list) {
 			tname = talloc_asprintf(trans, "%"PRIu64,
 						trans->generation);
-			if (!tname || !remember_string(hash, tname))
+			if (!tname || remember_string(hash, tname))
 				goto nomem;
 
 			list_for_each_entry(i, &trans->accessed, list) {
 				if (!i->ta_node)
 					continue;
-				if (!remember_string(hash, i->trans_name))
+				if (remember_string(hash, i->trans_name))
 					goto nomem;
 			}
 
-- 
2.35.3

[PATCH v3 05/16] tools/xenstore: make some write limit functions static

[Juergen Gross]

Some wrl_*() functions are only used in xenstored_domain.c, so make
them static. In order to avoid the need of forward declarations, move
the whole function block to the start of the file.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_domain.c | 456 +++++++++++++++---------------
 tools/xenstore/xenstored_domain.h |   3 -
 2 files changed, 228 insertions(+), 231 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 815f15cd1d..b21bdf6194 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -157,6 +157,234 @@ struct changed_domain
 
 static struct hashtable *domhash;
 
+static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
+static wrl_creditt wrl_config_rate           = WRL_RATE   * WRL_FACTOR;
+static wrl_creditt wrl_config_dburst         = WRL_DBURST * WRL_FACTOR;
+static wrl_creditt wrl_config_gburst         = WRL_GBURST * WRL_FACTOR;
+static wrl_creditt wrl_config_newdoms_dburst =
+	                         WRL_DBURST * WRL_NEWDOMS * WRL_FACTOR;
+
+long wrl_ntransactions;
+
+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;
+	int r;
+
+	r = clock_gettime(CLOCK_MONOTONIC, &now_ts);
+	if (r)
+		barf_perror("Could not find time (clock_gettime failed)");
+
+	now_wt->sec = now_ts.tv_sec;
+	now_wt->msec = now_ts.tv_nsec / 1000000;
+}
+
+static void wrl_xfer_credit(wrl_creditt *debit,  wrl_creditt debit_floor,
+			    wrl_creditt *credit, wrl_creditt credit_ceil)
+	/*
+	 * Transfers zero or more credit from "debit" to "credit".
+	 * Transfers as much as possible while maintaining
+	 * debit >= debit_floor and credit <= credit_ceil.
+	 * (If that's violated already, does nothing.)
+	 *
+	 * Sufficient conditions to avoid overflow, either of:
+	 *  |every argument| <= 0x3fffffff
+	 *  |every argument| <= 1E9
+	 *  |every argument| <= WRL_CREDIT_MAX
+	 * (And this condition is preserved.)
+	 */
+{
+	wrl_creditt xfer = MIN( *debit      - debit_floor,
+			        credit_ceil - *credit      );
+	if (xfer > 0) {
+		*debit -= xfer;
+		*credit += xfer;
+	}
+}
+
+static void wrl_domain_new(struct domain *domain)
+{
+	domain->wrl_credit = 0;
+	wrl_gettime_now(&domain->wrl_timestamp);
+	wrl_ndomains++;
+	/* Steal up to DBURST from the reserve */
+	wrl_xfer_credit(&wrl_reserve, -wrl_config_newdoms_dburst,
+			&domain->wrl_credit, wrl_config_dburst);
+}
+
+static void wrl_domain_destroy(struct domain *domain)
+{
+	wrl_ndomains--;
+	/*
+	 * Don't bother recalculating domain's credit - this just
+	 * means we don't give the reserve the ending domain's credit
+	 * for time elapsed since last update.
+	 */
+	wrl_xfer_credit(&domain->wrl_credit, 0,
+			&wrl_reserve, wrl_config_dburst);
+}
+
+static void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
+{
+	/*
+	 * We want to calculate
+	 *    credit += (now - timestamp) * RATE / ndoms;
+	 * But we want it to saturate, and to avoid floating point.
+	 * To avoid rounding errors from constantly adding small
+	 * amounts of credit, we only add credit for whole milliseconds.
+	 */
+	long seconds      = now.sec -  domain->wrl_timestamp.sec;
+	long milliseconds = now.msec - domain->wrl_timestamp.msec;
+	long msec;
+	int64_t denom, num;
+	wrl_creditt surplus;
+
+	seconds = MIN(seconds, 1000*1000); /* arbitrary, prevents overflow */
+	msec = seconds * 1000 + milliseconds;
+
+	if (msec < 0)
+                /* shouldn't happen with CLOCK_MONOTONIC */
+		msec = 0;
+
+	/* 32x32 -> 64 cannot overflow */
+	denom = (int64_t)msec * wrl_config_rate;
+	num  =  (int64_t)wrl_ndomains * 1000;
+	/* denom / num <= 1E6 * wrl_config_rate, so with
+	   reasonable wrl_config_rate, denom / num << 2^64 */
+
+	/* at last! */
+	domain->wrl_credit = MIN( (int64_t)domain->wrl_credit + denom / num,
+				  WRL_CREDIT_MAX );
+	/* (maybe briefly violating the DBURST cap on wrl_credit) */
+
+	/* maybe take from the reserve to make us nonnegative */
+	wrl_xfer_credit(&wrl_reserve,        0,
+			&domain->wrl_credit, 0);
+
+	/* return any surplus (over DBURST) to the reserve */
+	surplus = 0;
+	wrl_xfer_credit(&domain->wrl_credit, wrl_config_dburst,
+			&surplus,            WRL_CREDIT_MAX);
+	wrl_xfer_credit(&surplus,     0,
+			&wrl_reserve, wrl_config_gburst);
+	/* surplus is now implicitly discarded */
+
+	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);
+}
+
+void wrl_check_timeout(struct domain *domain,
+		       struct wrl_timestampt now,
+		       int *ptimeout)
+{
+	uint64_t num, denom;
+	int wakeup;
+
+	wrl_credit_update(domain, now);
+
+	if (domain->wrl_credit >= 0)
+		/* not blocked */
+		return;
+
+	if (!*ptimeout)
+		/* already decided on immediate wakeup,
+		   so no need to calculate our timeout */
+		return;
+
+	/* calculate  wakeup = now + -credit / (RATE / ndoms); */
+
+	/* credit cannot go more -ve than one transaction,
+	 * so the first multiplication cannot overflow even 32-bit */
+	num   = (uint64_t)(-domain->wrl_credit * 1000) * wrl_ndomains;
+	denom = wrl_config_rate;
+
+	wakeup = MIN( num / denom /* uint64_t */, INT_MAX );
+	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);
+}
+
+#define WRL_LOG(now, ...) \
+	(syslog(LOG_WARNING, "write rate limit: " __VA_ARGS__))
+
+void wrl_apply_debit_actual(struct domain *domain)
+{
+	struct wrl_timestampt now;
+
+	if (!domain || !domain_is_unprivileged(domain->conn))
+		/* sockets and privileged domain escape the write rate limit */
+		return;
+
+	wrl_gettime_now(&now);
+	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);
+
+	if (domain->wrl_credit < 0) {
+		if (!domain->wrl_delay_logged) {
+			domain->wrl_delay_logged = true;
+			WRL_LOG(now, "domain %ld is affected\n",
+				(long)domain->domid);
+		} else if (!wrl_log_last_warning) {
+			WRL_LOG(now, "rate limiting restarts\n");
+		}
+		wrl_log_last_warning = now.sec;
+	}
+}
+
+void wrl_log_periodic(struct wrl_timestampt now)
+{
+	if (wrl_log_last_warning &&
+	    (now.sec - wrl_log_last_warning) > WRL_LOGEVERY) {
+		WRL_LOG(now, "not in force recently\n");
+		wrl_log_last_warning = 0;
+	}
+}
+
+void wrl_apply_debit_direct(struct connection *conn)
+{
+	if (!conn)
+		/* some writes are generated internally */
+		return;
+
+	if (conn->transaction)
+		/* these are accounted for when the transaction ends */
+		return;
+
+	if (!wrl_ntransactions)
+		/* we don't conflict with anyone */
+		return;
+
+	wrl_apply_debit_actual(conn->domain);
+}
+
+void wrl_apply_debit_trans_commit(struct connection *conn)
+{
+	if (wrl_ntransactions <= 1)
+		/* our own transaction appears in the counter */
+		return;
+
+	wrl_apply_debit_actual(conn->domain);
+}
+
 static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
 {
 	return ((prod - cons) <= XENSTORE_RING_SIZE);
@@ -1446,234 +1674,6 @@ unsigned int domain_transaction_get(struct connection *conn)
 		: 0;
 }
 
-static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
-static wrl_creditt wrl_config_rate           = WRL_RATE   * WRL_FACTOR;
-static wrl_creditt wrl_config_dburst         = WRL_DBURST * WRL_FACTOR;
-static wrl_creditt wrl_config_gburst         = WRL_GBURST * WRL_FACTOR;
-static wrl_creditt wrl_config_newdoms_dburst =
-	                         WRL_DBURST * WRL_NEWDOMS * WRL_FACTOR;
-
-long wrl_ntransactions;
-
-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;
-	int r;
-
-	r = clock_gettime(CLOCK_MONOTONIC, &now_ts);
-	if (r)
-		barf_perror("Could not find time (clock_gettime failed)");
-
-	now_wt->sec = now_ts.tv_sec;
-	now_wt->msec = now_ts.tv_nsec / 1000000;
-}
-
-static void wrl_xfer_credit(wrl_creditt *debit,  wrl_creditt debit_floor,
-			    wrl_creditt *credit, wrl_creditt credit_ceil)
-	/*
-	 * Transfers zero or more credit from "debit" to "credit".
-	 * Transfers as much as possible while maintaining
-	 * debit >= debit_floor and credit <= credit_ceil.
-	 * (If that's violated already, does nothing.)
-	 *
-	 * Sufficient conditions to avoid overflow, either of:
-	 *  |every argument| <= 0x3fffffff
-	 *  |every argument| <= 1E9
-	 *  |every argument| <= WRL_CREDIT_MAX
-	 * (And this condition is preserved.)
-	 */
-{
-	wrl_creditt xfer = MIN( *debit      - debit_floor,
-			        credit_ceil - *credit      );
-	if (xfer > 0) {
-		*debit -= xfer;
-		*credit += xfer;
-	}
-}
-
-void wrl_domain_new(struct domain *domain)
-{
-	domain->wrl_credit = 0;
-	wrl_gettime_now(&domain->wrl_timestamp);
-	wrl_ndomains++;
-	/* Steal up to DBURST from the reserve */
-	wrl_xfer_credit(&wrl_reserve, -wrl_config_newdoms_dburst,
-			&domain->wrl_credit, wrl_config_dburst);
-}
-
-void wrl_domain_destroy(struct domain *domain)
-{
-	wrl_ndomains--;
-	/*
-	 * Don't bother recalculating domain's credit - this just
-	 * means we don't give the reserve the ending domain's credit
-	 * for time elapsed since last update.
-	 */
-	wrl_xfer_credit(&domain->wrl_credit, 0,
-			&wrl_reserve, wrl_config_dburst);
-}
-
-void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
-{
-	/*
-	 * We want to calculate
-	 *    credit += (now - timestamp) * RATE / ndoms;
-	 * But we want it to saturate, and to avoid floating point.
-	 * To avoid rounding errors from constantly adding small
-	 * amounts of credit, we only add credit for whole milliseconds.
-	 */
-	long seconds      = now.sec -  domain->wrl_timestamp.sec;
-	long milliseconds = now.msec - domain->wrl_timestamp.msec;
-	long msec;
-	int64_t denom, num;
-	wrl_creditt surplus;
-
-	seconds = MIN(seconds, 1000*1000); /* arbitrary, prevents overflow */
-	msec = seconds * 1000 + milliseconds;
-
-	if (msec < 0)
-                /* shouldn't happen with CLOCK_MONOTONIC */
-		msec = 0;
-
-	/* 32x32 -> 64 cannot overflow */
-	denom = (int64_t)msec * wrl_config_rate;
-	num  =  (int64_t)wrl_ndomains * 1000;
-	/* denom / num <= 1E6 * wrl_config_rate, so with
-	   reasonable wrl_config_rate, denom / num << 2^64 */
-
-	/* at last! */
-	domain->wrl_credit = MIN( (int64_t)domain->wrl_credit + denom / num,
-				  WRL_CREDIT_MAX );
-	/* (maybe briefly violating the DBURST cap on wrl_credit) */
-
-	/* maybe take from the reserve to make us nonnegative */
-	wrl_xfer_credit(&wrl_reserve,        0,
-			&domain->wrl_credit, 0);
-
-	/* return any surplus (over DBURST) to the reserve */
-	surplus = 0;
-	wrl_xfer_credit(&domain->wrl_credit, wrl_config_dburst,
-			&surplus,            WRL_CREDIT_MAX);
-	wrl_xfer_credit(&surplus,     0,
-			&wrl_reserve, wrl_config_gburst);
-	/* surplus is now implicitly discarded */
-
-	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);
-}
-
-void wrl_check_timeout(struct domain *domain,
-		       struct wrl_timestampt now,
-		       int *ptimeout)
-{
-	uint64_t num, denom;
-	int wakeup;
-
-	wrl_credit_update(domain, now);
-
-	if (domain->wrl_credit >= 0)
-		/* not blocked */
-		return;
-
-	if (!*ptimeout)
-		/* already decided on immediate wakeup,
-		   so no need to calculate our timeout */
-		return;
-
-	/* calculate  wakeup = now + -credit / (RATE / ndoms); */
-
-	/* credit cannot go more -ve than one transaction,
-	 * so the first multiplication cannot overflow even 32-bit */
-	num   = (uint64_t)(-domain->wrl_credit * 1000) * wrl_ndomains;
-	denom = wrl_config_rate;
-
-	wakeup = MIN( num / denom /* uint64_t */, INT_MAX );
-	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);
-}
-
-#define WRL_LOG(now, ...) \
-	(syslog(LOG_WARNING, "write rate limit: " __VA_ARGS__))
-
-void wrl_apply_debit_actual(struct domain *domain)
-{
-	struct wrl_timestampt now;
-
-	if (!domain || !domid_is_unprivileged(domain->domid))
-		/* sockets and privileged domain escape the write rate limit */
-		return;
-
-	wrl_gettime_now(&now);
-	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);
-
-	if (domain->wrl_credit < 0) {
-		if (!domain->wrl_delay_logged) {
-			domain->wrl_delay_logged = true;
-			WRL_LOG(now, "domain %ld is affected\n",
-				(long)domain->domid);
-		} else if (!wrl_log_last_warning) {
-			WRL_LOG(now, "rate limiting restarts\n");
-		}
-		wrl_log_last_warning = now.sec;
-	}
-}
-
-void wrl_log_periodic(struct wrl_timestampt now)
-{
-	if (wrl_log_last_warning &&
-	    (now.sec - wrl_log_last_warning) > WRL_LOGEVERY) {
-		WRL_LOG(now, "not in force recently\n");
-		wrl_log_last_warning = 0;
-	}
-}
-
-void wrl_apply_debit_direct(struct connection *conn)
-{
-	if (!conn)
-		/* some writes are generated internally */
-		return;
-
-	if (conn->transaction)
-		/* these are accounted for when the transaction ends */
-		return;
-
-	if (!wrl_ntransactions)
-		/* we don't conflict with anyone */
-		return;
-
-	wrl_apply_debit_actual(conn->domain);
-}
-
-void wrl_apply_debit_trans_commit(struct connection *conn)
-{
-	if (wrl_ntransactions <= 1)
-		/* our own transaction appears in the counter */
-		return;
-
-	wrl_apply_debit_actual(conn->domain);
-}
-
 const char *dump_state_connections(FILE *fp)
 {
 	const char *ret = NULL;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 5123453f6c..89be643de4 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -163,9 +163,6 @@ struct wrl_timestampt {
 extern long wrl_ntransactions;
 
 void wrl_gettime_now(struct wrl_timestampt *now_ts);
-void wrl_domain_new(struct domain *domain);
-void wrl_domain_destroy(struct domain *domain);
-void wrl_credit_update(struct domain *domain, struct wrl_timestampt now);
 void wrl_check_timeout(struct domain *domain,
                        struct wrl_timestampt now,
                        int *ptimeout);
-- 
2.35.3

[PATCH v3 06/16] tools/xenstore: switch write limiting to use millisecond time base

[Juergen Gross]

There is no need to keep struct wrl_timestampt, as it serves the same
purpose as the more simple time base provided by get_now().

Move some more stuff from xenstored_domain.h into xenstored_domain.c
as it is being used nowhere else.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c   |  8 ++---
 tools/xenstore/xenstored_core.h   |  7 ++--
 tools/xenstore/xenstored_domain.c | 56 +++++++++++++------------------
 tools/xenstore/xenstored_domain.h | 21 ++----------
 4 files changed, 32 insertions(+), 60 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c467a704a1..e8f46495de 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -192,7 +192,7 @@ void reopen_log(void)
 	}
 }
 
-static uint64_t get_now_msec(void)
+uint64_t get_now_msec(void)
 {
 	struct timespec now_ts;
 
@@ -510,7 +510,6 @@ fail:
 static void initialize_fds(int *p_sock_pollfd_idx, int *ptimeout)
 {
 	struct connection *conn;
-	struct wrl_timestampt now;
 	uint64_t msecs;
 
 	if (fds)
@@ -530,13 +529,12 @@ static void initialize_fds(int *p_sock_pollfd_idx, int *ptimeout)
 		xce_pollfd_idx = set_fd(xenevtchn_fd(xce_handle),
 					POLLIN|POLLPRI);
 
-	wrl_gettime_now(&now);
-	wrl_log_periodic(now);
 	msecs = get_now_msec();
+	wrl_log_periodic(msecs);
 
 	list_for_each_entry(conn, &connections, list) {
 		if (conn->domain) {
-			wrl_check_timeout(conn->domain, now, ptimeout);
+			wrl_check_timeout(conn->domain, msecs, ptimeout);
 			check_event_timeout(conn, msecs, ptimeout);
 			if (conn_can_read(conn) ||
 			    (conn_can_write(conn) &&
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 92d5b50f3c..84a611cbb5 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -47,10 +47,6 @@
 /* DEFAULT_BUFFER_SIZE should be large enough for each errno string. */
 #define DEFAULT_BUFFER_SIZE 16
 
-typedef int32_t wrl_creditt;
-#define WRL_CREDIT_MAX (1000*1000*1000)
-/* ^ satisfies non-overflow condition for wrl_xfer_credit */
-
 struct xs_state_connection;
 
 struct buffered_data
@@ -320,6 +316,9 @@ extern bool keep_orphans;
 
 extern unsigned int timeout_watch_event_msec;
 
+/* Get internal time in milliseconds. */
+uint64_t get_now_msec(void);
+
 /* Map the kernel's xenstore page. */
 void *xenbus_map(void);
 void unmap_xenbus(void *interface);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index b21bdf6194..60d3aa1ddb 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -99,6 +99,8 @@ struct quota soft_quotas[ACC_N] = {
 	},
 };
 
+typedef int32_t wrl_creditt;
+
 struct domain
 {
 	/* The id of this domain */
@@ -139,7 +141,7 @@ struct domain
 
 	/* write rate limit */
 	wrl_creditt wrl_credit; /* [ -wrl_config_writecost, +_dburst ] */
-	struct wrl_timestampt wrl_timestamp;
+	uint64_t wrl_timestamp;
 	bool wrl_delay_logged;
 };
 
@@ -157,6 +159,17 @@ struct changed_domain
 
 static struct hashtable *domhash;
 
+/* Write rate limiting */
+
+/* Satisfies non-overflow condition for wrl_xfer_credit. */
+#define WRL_CREDIT_MAX (1000*1000*1000)
+#define WRL_FACTOR     1000 /* for fixed-point arithmetic */
+#define WRL_RATE        200
+#define WRL_DBURST       10
+#define WRL_GBURST     1000
+#define WRL_NEWDOMS       5
+#define WRL_LOGEVERY    120 /* seconds */
+
 static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
 static wrl_creditt wrl_config_rate           = WRL_RATE   * WRL_FACTOR;
 static wrl_creditt wrl_config_dburst         = WRL_DBURST * WRL_FACTOR;
@@ -176,19 +189,6 @@ do {						\
 		trace("wrl: " __VA_ARGS__);	\
 } while (0)
 
-void wrl_gettime_now(struct wrl_timestampt *now_wt)
-{
-	struct timespec now_ts;
-	int r;
-
-	r = clock_gettime(CLOCK_MONOTONIC, &now_ts);
-	if (r)
-		barf_perror("Could not find time (clock_gettime failed)");
-
-	now_wt->sec = now_ts.tv_sec;
-	now_wt->msec = now_ts.tv_nsec / 1000000;
-}
-
 static void wrl_xfer_credit(wrl_creditt *debit,  wrl_creditt debit_floor,
 			    wrl_creditt *credit, wrl_creditt credit_ceil)
 	/*
@@ -215,7 +215,7 @@ static void wrl_xfer_credit(wrl_creditt *debit,  wrl_creditt debit_floor,
 static void wrl_domain_new(struct domain *domain)
 {
 	domain->wrl_credit = 0;
-	wrl_gettime_now(&domain->wrl_timestamp);
+	domain->wrl_timestamp = get_now_msec();
 	wrl_ndomains++;
 	/* Steal up to DBURST from the reserve */
 	wrl_xfer_credit(&wrl_reserve, -wrl_config_newdoms_dburst,
@@ -234,7 +234,7 @@ static void wrl_domain_destroy(struct domain *domain)
 			&wrl_reserve, wrl_config_dburst);
 }
 
-static void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
+static void wrl_credit_update(struct domain *domain, uint64_t now)
 {
 	/*
 	 * We want to calculate
@@ -243,18 +243,12 @@ static void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
 	 * To avoid rounding errors from constantly adding small
 	 * amounts of credit, we only add credit for whole milliseconds.
 	 */
-	long seconds      = now.sec -  domain->wrl_timestamp.sec;
-	long milliseconds = now.msec - domain->wrl_timestamp.msec;
 	long msec;
 	int64_t denom, num;
 	wrl_creditt surplus;
 
-	seconds = MIN(seconds, 1000*1000); /* arbitrary, prevents overflow */
-	msec = seconds * 1000 + milliseconds;
-
-	if (msec < 0)
-                /* shouldn't happen with CLOCK_MONOTONIC */
-		msec = 0;
+	/* Prevent overflow by limiting to 32 bits. */
+	msec = MIN(now - domain->wrl_timestamp, 1000 * 1000 * 1000);
 
 	/* 32x32 -> 64 cannot overflow */
 	denom = (int64_t)msec * wrl_config_rate;
@@ -286,9 +280,7 @@ static void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
 		  (long)wrl_reserve, (long)surplus);
 }
 
-void wrl_check_timeout(struct domain *domain,
-		       struct wrl_timestampt now,
-		       int *ptimeout)
+void wrl_check_timeout(struct domain *domain, uint64_t now, int *ptimeout)
 {
 	uint64_t num, denom;
 	int wakeup;
@@ -325,13 +317,13 @@ void wrl_check_timeout(struct domain *domain,
 
 void wrl_apply_debit_actual(struct domain *domain)
 {
-	struct wrl_timestampt now;
+	uint64_t now;
 
 	if (!domain || !domain_is_unprivileged(domain->conn))
 		/* sockets and privileged domain escape the write rate limit */
 		return;
 
-	wrl_gettime_now(&now);
+	now = get_now_msec();
 	wrl_credit_update(domain, now);
 
 	domain->wrl_credit -= wrl_config_writecost;
@@ -346,14 +338,14 @@ void wrl_apply_debit_actual(struct domain *domain)
 		} else if (!wrl_log_last_warning) {
 			WRL_LOG(now, "rate limiting restarts\n");
 		}
-		wrl_log_last_warning = now.sec;
+		wrl_log_last_warning = now / 1000;
 	}
 }
 
-void wrl_log_periodic(struct wrl_timestampt now)
+void wrl_log_periodic(uint64_t now)
 {
 	if (wrl_log_last_warning &&
-	    (now.sec - wrl_log_last_warning) > WRL_LOGEVERY) {
+	    (now / 1000 - wrl_log_last_warning) > WRL_LOGEVERY) {
 		WRL_LOG(now, "not in force recently\n");
 		wrl_log_last_warning = 0;
 	}
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 89be643de4..bf63f3fcc6 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -146,27 +146,10 @@ void domain_reset_global_acc(void);
 bool domain_max_chk(const struct connection *conn, unsigned int what,
 		    unsigned int val);
 
-/* Write rate limiting */
-
-#define WRL_FACTOR   1000 /* for fixed-point arithmetic */
-#define WRL_RATE      200
-#define WRL_DBURST     10
-#define WRL_GBURST   1000
-#define WRL_NEWDOMS     5
-#define WRL_LOGEVERY  120 /* seconds */
-
-struct wrl_timestampt {
-	time_t sec;
-	int msec;
-};
-
 extern long wrl_ntransactions;
 
-void wrl_gettime_now(struct wrl_timestampt *now_ts);
-void wrl_check_timeout(struct domain *domain,
-                       struct wrl_timestampt now,
-                       int *ptimeout);
-void wrl_log_periodic(struct wrl_timestampt now);
+void wrl_check_timeout(struct domain *domain, uint64_t now, int *ptimeout);
+void wrl_log_periodic(uint64_t now);
 void wrl_apply_debit_direct(struct connection *conn);
 void wrl_apply_debit_trans_commit(struct connection *conn);
 
-- 
2.35.3

[PATCH v3 07/16] tools/xenstore: remove stale TODO file

[Juergen Gross]

The TODO file is not really helpful any longer. It contains only
entries which no longer apply or it is unknown what they are meant
for ("Dynamic/supply nodes", "Remove assumption that rename doesn't
fail").

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/TODO | 10 ----------
 1 file changed, 10 deletions(-)
 delete mode 100644 tools/xenstore/TODO

diff --git a/tools/xenstore/TODO b/tools/xenstore/TODO
deleted file mode 100644
index 71d5bbbf50..0000000000
--- a/tools/xenstore/TODO
+++ /dev/null
@@ -1,10 +0,0 @@
-TODO in no particular order.  Some of these will never be done.  There
-are omissions of important but necessary things.  It is up to the
-reader to fill in the blanks.
-
-- Timeout failed watch responses
-- Dynamic/supply nodes
-- Persistant storage of introductions, watches and transactions, so daemon can restart
-- Remove assumption that rename doesn't fail
-- Multi-root transactions, for setting up front and back ends at same time.
-
-- 
2.35.3

[PATCH v3 08/16] tools/xenstore: remove unused events list

[Juergen Gross]

struct watch contains an unused struct list_head events. Remove it.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_watch.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index e8eb35de02..4195c59e17 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -36,9 +36,6 @@ struct watch
 	/* Watches on this connection */
 	struct list_head list;
 
-	/* Current outstanding events applying to this watch. */
-	struct list_head events;
-
 	/* Offset into path for skipping prefix (used for relative paths). */
 	unsigned int prefix_len;
 
@@ -205,8 +202,6 @@ static struct watch *add_watch(struct connection *conn, char *path, char *token,
 
 	watch->prefix_len = relative ? strlen(get_implicit_path(conn)) + 1 : 0;
 
-	INIT_LIST_HEAD(&watch->events);
-
 	domain_watch_inc(conn);
 	list_add_tail(&watch->list, &conn->watches);
 	talloc_set_destructor(watch, destroy_watch);
-- 
2.35.3

[PATCH v3 09/16] tools/xenstore: remove support of file backed data base

[Juergen Gross]

In order to prepare the replacement of TDB with direct accessible nodes
in memory, remove the support for a file backed data base.

This allows to remove xs_tdb_dump, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 .gitignore                      |  1 -
 tools/xenstore/Makefile         |  5 +-
 tools/xenstore/xenstored_core.c | 18 ++-----
 tools/xenstore/xs_tdb_dump.c    | 86 ---------------------------------
 4 files changed, 4 insertions(+), 106 deletions(-)
 delete mode 100644 tools/xenstore/xs_tdb_dump.c

diff --git a/.gitignore b/.gitignore
index beac034784..26f532e77c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -248,7 +248,6 @@ tools/xenstore/xenstore-rm
 tools/xenstore/xenstore-watch
 tools/xenstore/xenstore-write
 tools/xenstore/xenstored
-tools/xenstore/xs_tdb_dump
 tools/xentop/xentop
 tools/xentrace/xentrace_setsize
 tools/xentrace/tbctl
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index ce7a68178f..56723139a1 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -29,7 +29,7 @@ CLIENTS += xenstore-write xenstore-ls xenstore-watch
 
 TARGETS := xenstore $(CLIENTS) xenstore-control
 ifeq ($(XENSTORE_XENSTORED),y)
-TARGETS += xs_tdb_dump xenstored
+TARGETS += xenstored
 endif
 
 .PHONY: all
@@ -50,9 +50,6 @@ xenstore: xenstore_client.o xs_lib.o
 xenstore-control: xenstore_control.o
 	$(CC) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(APPEND_LDFLAGS)
 
-xs_tdb_dump: xs_tdb_dump.o utils.o tdb.o talloc.o
-	$(CC) $(LDFLAGS) $^ -o $@ $(APPEND_LDFLAGS)
-
 .PHONY: clean
 clean::
 	$(RM) $(TARGETS) $(DEPS_RM)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index e8f46495de..07fbac55ac 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2301,8 +2301,6 @@ static void accept_connection(int sock)
 }
 #endif
 
-static int tdb_flags = TDB_INTERNAL | TDB_NOLOCK;
-
 /* We create initial nodes manually. */
 static void manual_node(const char *name, const char *child)
 {
@@ -2354,14 +2352,11 @@ void setup_structure(bool live_update)
 {
 	char *tdbname;
 
-	tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
+	tdbname = talloc_strdup(talloc_autofree_context(), "/dev/mem");
 	if (!tdbname)
 		barf_perror("Could not create tdbname");
 
-	if (!(tdb_flags & TDB_INTERNAL))
-		unlink(tdbname);
-
-	tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags,
+	tdb_ctx = tdb_open_ex(tdbname, 7919, TDB_INTERNAL | TDB_NOLOCK,
 			      O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC,
 			      0640, &tdb_logger, NULL);
 	if (!tdb_ctx)
@@ -2659,8 +2654,6 @@ static void usage(void)
 "                          watch-event: time a watch-event is kept pending\n"
 "  -R, --no-recovery       to request that no recovery should be attempted when\n"
 "                          the store is corrupted (debug only),\n"
-"  -I, --internal-db [on|off] store database in memory, not on disk, default is\n"
-"                          memory, with \"--internal-db off\" it is on disk\n"
 "  -K, --keep-orphans      don't delete nodes owned by a domain when the\n"
 "                          domain is deleted (this is a security risk!)\n"
 "  -V, --verbose           to request verbose execution.\n");
@@ -2687,7 +2680,6 @@ static struct option options[] = {
 	{ "quota-soft", 1, NULL, 'q' },
 	{ "timeout", 1, NULL, 'w' },
 	{ "no-recovery", 0, NULL, 'R' },
-	{ "internal-db", 2, NULL, 'I' },
 	{ "keep-orphans", 0, NULL, 'K' },
 	{ "verbose", 0, NULL, 'V' },
 	{ "watch-nb", 1, NULL, 'W' },
@@ -2811,7 +2803,7 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "DE:F:HI::KNPS:t:A:M:Q:q:T:RVW:w:U",
+				  "DE:F:H::KNPS:t:A:M:Q:q:T:RVW:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
 		case 'D':
@@ -2848,10 +2840,6 @@ int main(int argc, char *argv[])
 			if (set_trace_switch(optarg))
 				barf("Illegal trace switch \"%s\"\n", optarg);
 			break;
-		case 'I':
-			if (optarg && !strcmp(optarg, "off"))
-				tdb_flags = 0;
-			break;
 		case 'K':
 			keep_orphans = true;
 			break;
diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c
deleted file mode 100644
index 5d2db392b4..0000000000
--- a/tools/xenstore/xs_tdb_dump.c
+++ /dev/null
@@ -1,86 +0,0 @@
-/* Simple program to dump out all records of TDB */
-#include <stdint.h>
-#include <stdlib.h>
-#include <fcntl.h>
-#include <stdio.h>
-#include <stdarg.h>
-#include <string.h>
-#include <sys/types.h>
-#include "xenstore_lib.h"
-#include "tdb.h"
-#include "talloc.h"
-#include "utils.h"
-
-static uint32_t total_size(struct xs_tdb_record_hdr *hdr)
-{
-	return sizeof(*hdr) + hdr->num_perms * sizeof(struct xs_permissions) 
-		+ hdr->datalen + hdr->childlen;
-}
-
-static char perm_to_char(unsigned int perm)
-{
-	return perm == XS_PERM_READ ? 'r' :
-		perm == XS_PERM_WRITE ? 'w' :
-		perm == XS_PERM_NONE ? '-' :
-		perm == (XS_PERM_READ|XS_PERM_WRITE) ? 'b' :
-		'?';
-}
-
-static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
-	va_end(ap);
-}
-
-int main(int argc, char *argv[])
-{
-	TDB_DATA key;
-	TDB_CONTEXT *tdb;
-
-	if (argc != 2)
-		barf("Usage: xs_tdb_dump <tdbfile>");
-
-	tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0,
-			  &tdb_logger, NULL);
-	if (!tdb)
-		barf_perror("Could not open %s", argv[1]);
-
-	key = tdb_firstkey(tdb);
-	while (key.dptr) {
-		TDB_DATA data;
-		struct xs_tdb_record_hdr *hdr;
-
-		data = tdb_fetch(tdb, key);
-		hdr = (void *)data.dptr;
-		if (data.dsize < sizeof(*hdr))
-			fprintf(stderr, "%.*s: BAD truncated\n",
-				(int)key.dsize, key.dptr);
-		else if (data.dsize != total_size(hdr))
-			fprintf(stderr, "%.*s: BAD length %zu for %u/%u/%u (%u)\n",
-				(int)key.dsize, key.dptr, data.dsize,
-				hdr->num_perms, hdr->datalen,
-				hdr->childlen, total_size(hdr));
-		else {
-			unsigned int i;
-			char *p;
-
-			printf("%.*s: ", (int)key.dsize, key.dptr);
-			for (i = 0; i < hdr->num_perms; i++)
-				printf("%s%c%u",
-				       i == 0 ? "" : ",",
-				       perm_to_char(hdr->perms[i].perms),
-				       hdr->perms[i].id);
-			p = (void *)&hdr->perms[hdr->num_perms];
-			printf(" %.*s\n", hdr->datalen, p);
-			p += hdr->datalen;
-			for (i = 0; i < hdr->childlen; i += strlen(p+i)+1)
-				printf("\t-> %s\n", p+i);
-		}
-		key = tdb_nextkey(tdb, key);
-	}
-	return 0;
-}
-
-- 
2.35.3

[PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Juergen Gross]

Replace the usage of the xenstore private list.h header with the
common xen_list.h one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 tools/libs/store/xs.c | 56 +++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 7a9a8b1656..3813b69ae2 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -35,13 +35,13 @@
 #include <errno.h>
 #include "xenstore.h"
 #include "xs_lib.h"
-#include "list.h"
 #include "utils.h"
 
 #include <xentoolcore_internal.h>
+#include <xen_list.h>
 
 struct xs_stored_msg {
-	struct list_head list;
+	XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
 	struct xsd_sockmsg hdr;
 	char *body;
 };
@@ -70,7 +70,7 @@ struct xs_handle {
          * A list of fired watch messages, protected by a mutex. Users can
          * wait on the conditional variable until a watch is pending.
          */
-	struct list_head watch_list;
+	XEN_TAILQ_HEAD(, struct xs_stored_msg) watch_list;
 	pthread_mutex_t watch_mutex;
 	pthread_cond_t watch_condvar;
 
@@ -84,7 +84,7 @@ struct xs_handle {
          * because we serialise requests. The requester can wait on the
          * conditional variable for its response.
          */
-	struct list_head reply_list;
+	XEN_TAILQ_HEAD(, struct xs_stored_msg) reply_list;
 	pthread_mutex_t reply_mutex;
 	pthread_cond_t reply_condvar;
 
@@ -133,8 +133,8 @@ static void *read_thread(void *arg);
 struct xs_handle {
 	int fd;
 	Xentoolcore__Active_Handle tc_ah; /* for restrict */
-	struct list_head reply_list;
-	struct list_head watch_list;
+	XEN_TAILQ_HEAD(, struct xs_stored_msg) reply_list;
+	XEN_TAILQ_HEAD(, struct xs_stored_msg) watch_list;
 	/* Clients can select() on this pipe to wait for a watch to fire. */
 	int watch_pipe[2];
 	/* Filtering watch event in unwatch function? */
@@ -180,7 +180,7 @@ int xs_fileno(struct xs_handle *h)
 
 	if ((h->watch_pipe[0] == -1) && (pipe(h->watch_pipe) != -1)) {
 		/* Kick things off if the watch list is already non-empty. */
-		if (!list_empty(&h->watch_list))
+		if (!XEN_TAILQ_EMPTY(&h->watch_list))
 			while (write(h->watch_pipe[1], &c, 1) != 1)
 				continue;
 	}
@@ -262,8 +262,8 @@ static struct xs_handle *get_handle(const char *connect_to)
 	if (h->fd == -1)
 		goto err;
 
-	INIT_LIST_HEAD(&h->reply_list);
-	INIT_LIST_HEAD(&h->watch_list);
+	XEN_TAILQ_INIT(&h->reply_list);
+	XEN_TAILQ_INIT(&h->watch_list);
 
 	/* Watch pipe is allocated on demand in xs_fileno(). */
 	h->watch_pipe[0] = h->watch_pipe[1] = -1;
@@ -329,12 +329,12 @@ struct xs_handle *xs_open(unsigned long flags)
 static void close_free_msgs(struct xs_handle *h) {
 	struct xs_stored_msg *msg, *tmsg;
 
-	list_for_each_entry_safe(msg, tmsg, &h->reply_list, list) {
+	XEN_TAILQ_FOREACH_SAFE(msg, &h->reply_list, list, tmsg) {
 		free(msg->body);
 		free(msg);
 	}
 
-	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
+	XEN_TAILQ_FOREACH_SAFE(msg, &h->watch_list, list, tmsg) {
 		free(msg->body);
 		free(msg);
 	}
@@ -459,17 +459,17 @@ static void *read_reply(
 
 	mutex_lock(&h->reply_mutex);
 #ifdef USE_PTHREAD
-	while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
+	while (XEN_TAILQ_EMPTY(&h->reply_list) && read_from_thread && h->fd != -1)
 		condvar_wait(&h->reply_condvar, &h->reply_mutex);
 #endif
-	if (list_empty(&h->reply_list)) {
+	if (XEN_TAILQ_EMPTY(&h->reply_list)) {
 		mutex_unlock(&h->reply_mutex);
 		errno = EINVAL;
 		return NULL;
 	}
-	msg = list_top(&h->reply_list, struct xs_stored_msg, list);
-	list_del(&msg->list);
-	assert(list_empty(&h->reply_list));
+	msg = XEN_TAILQ_FIRST(&h->reply_list);
+	XEN_TAILQ_REMOVE(&h->reply_list, msg, list);
+	assert(XEN_TAILQ_EMPTY(&h->reply_list));
 	mutex_unlock(&h->reply_mutex);
 
 	*type = msg->hdr.type;
@@ -883,7 +883,7 @@ static void xs_maybe_clear_watch_pipe(struct xs_handle *h)
 {
 	char c;
 
-	if (list_empty(&h->watch_list) && (h->watch_pipe[0] != -1))
+	if (XEN_TAILQ_EMPTY(&h->watch_list) && (h->watch_pipe[0] != -1))
 		while (read(h->watch_pipe[0], &c, 1) != 1)
 			continue;
 }
@@ -907,7 +907,7 @@ static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
 	 * we haven't called xs_watch.	Presumably the application
 	 * will do so later; in the meantime we just block.
 	 */
-	while (list_empty(&h->watch_list) && h->fd != -1) {
+	while (XEN_TAILQ_EMPTY(&h->watch_list) && h->fd != -1) {
 		if (nonblocking) {
 			mutex_unlock(&h->watch_mutex);
 			errno = EAGAIN;
@@ -925,13 +925,13 @@ static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
 
 #endif /* !defined(USE_PTHREAD) */
 
-	if (list_empty(&h->watch_list)) {
+	if (XEN_TAILQ_EMPTY(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
 		return NULL;
 	}
-	msg = list_top(&h->watch_list, struct xs_stored_msg, list);
-	list_del(&msg->list);
+	msg = XEN_TAILQ_FIRST(&h->watch_list);
+	XEN_TAILQ_REMOVE(&h->watch_list, msg, list);
 
 	xs_maybe_clear_watch_pipe(h);
 	mutex_unlock(&h->watch_mutex);
@@ -1007,12 +1007,12 @@ bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
 	/* Filter the watch list to remove potential message */
 	mutex_lock(&h->watch_mutex);
 
-	if (list_empty(&h->watch_list)) {
+	if (XEN_TAILQ_EMPTY(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		return res;
 	}
 
-	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
+	XEN_TAILQ_FOREACH_SAFE(msg, &h->watch_list, list, tmsg) {
 		assert(msg->hdr.type == XS_WATCH_EVENT);
 
 		s = msg->body;
@@ -1034,7 +1034,7 @@ bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
 
 		if (l_token && !strcmp(token, l_token) &&
 		    l_path && xs_path_is_subpath(path, l_path)) {
-			list_del(&msg->list);
+			XEN_TAILQ_REMOVE(&h->watch_list, msg, list);
 			free(msg);
 		}
 	}
@@ -1290,12 +1290,12 @@ static int read_message(struct xs_handle *h, int nonblocking)
 		cleanup_push(pthread_mutex_unlock, &h->watch_mutex);
 
 		/* Kick users out of their select() loop. */
-		if (list_empty(&h->watch_list) &&
+		if (XEN_TAILQ_EMPTY(&h->watch_list) &&
 		    (h->watch_pipe[1] != -1))
 			while (write(h->watch_pipe[1], body, 1) != 1) /* Cancellation point */
 				continue;
 
-		list_add_tail(&msg->list, &h->watch_list);
+		XEN_TAILQ_INSERT_TAIL(&h->watch_list, msg, list);
 
 		condvar_signal(&h->watch_condvar);
 
@@ -1304,13 +1304,13 @@ static int read_message(struct xs_handle *h, int nonblocking)
 		mutex_lock(&h->reply_mutex);
 
 		/* There should only ever be one response pending! */
-		if (!list_empty(&h->reply_list)) {
+		if (!XEN_TAILQ_EMPTY(&h->reply_list)) {
 			mutex_unlock(&h->reply_mutex);
 			saved_errno = EEXIST;
 			goto error_freebody;
 		}
 
-		list_add_tail(&msg->list, &h->reply_list);
+		XEN_TAILQ_INSERT_TAIL(&h->reply_list, msg, list);
 		condvar_signal(&h->reply_condvar);
 
 		mutex_unlock(&h->reply_mutex);
-- 
2.35.3

[PATCH v3 11/16] tools/libs/store: make libxenstore independent of utils.h

[Juergen Gross]

There is no real need for including tools/xenstore/utils.h from
libxenstore, as only streq() and ARRAY_SIZE() are obtained via that
header.

streq() is just !strcmp(), and ARRAY_SIZE() is brought in via
xen-tools/common-macros.h.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 tools/libs/store/xs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 3813b69ae2..76ffb1be45 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -33,9 +33,9 @@
 #include <signal.h>
 #include <stdint.h>
 #include <errno.h>
+#include <xen-tools/common-macros.h>
 #include "xenstore.h"
 #include "xs_lib.h"
-#include "utils.h"
 
 #include <xentoolcore_internal.h>
 #include <xen_list.h>
@@ -437,7 +437,7 @@ static int get_error(const char *errorstring)
 {
 	unsigned int i;
 
-	for (i = 0; !streq(errorstring, xsd_errors[i].errstring); i++)
+	for (i = 0; strcmp(errorstring, xsd_errors[i].errstring); i++)
 		if (i == ARRAY_SIZE(xsd_errors) - 1)
 			return EINVAL;
 	return xsd_errors[i].errnum;
-- 
2.35.3

[PATCH v3 12/16] tools/xenstore: remove no longer needed functions from xs_lib.c

[Juergen Gross]

xs_daemon_tdb() in xs_lib.c is no longer used at all, so it can be
removed. xs_domain_dev() and xs_write_all() are not used by xenstored,
so they can be moved to tools/libs/store/xs.c.

xs_daemon_rootdir() is used by xenstored only and it only calls
xs_daemon_rundir(), so replace its use cases with xs_daemon_rundir()
and remove it from xs_lib.c.

xs_daemon_socket_ro() is needed in libxenstore only, so move it to
tools/libs/store/xs.c.

Move functions used by xenstore-client only to xenstore_client.c.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- move xs_write_all(), too
V3:
- remove xs_daemon_rootdir(), move xs_daemon_socket_ro()
---
 tools/include/xenstore.h           |   3 +
 tools/include/xenstore_lib.h       |   3 -
 tools/libs/store/xs.c              |  43 ++++++++
 tools/xenstore/xenstore_client.c   | 129 ++++++++++++++++++++++
 tools/xenstore/xenstored_control.c |   4 +-
 tools/xenstore/xenstored_core.c    |   3 +-
 tools/xenstore/xs_lib.c            | 166 -----------------------------
 tools/xenstore/xs_lib.h            |  19 ----
 8 files changed, 178 insertions(+), 192 deletions(-)

diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
index 2b3f69fb61..a442252849 100644
--- a/tools/include/xenstore.h
+++ b/tools/include/xenstore.h
@@ -113,6 +113,9 @@ void *xs_read(struct xs_handle *h, xs_transaction_t t,
 bool xs_write(struct xs_handle *h, xs_transaction_t t,
 	      const char *path, const void *data, unsigned int len);
 
+/* Simple write function: loops for you. */
+bool xs_write_all(int fd, const void *data, unsigned int len);
+
 /* Create a new directory.
  * Returns false on failure, or success if it already exists.
  */
diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
index 2266009ec1..43eec87379 100644
--- a/tools/include/xenstore_lib.h
+++ b/tools/include/xenstore_lib.h
@@ -47,9 +47,6 @@ const char *xs_daemon_rundir(void);
 const char *xs_daemon_socket(void);
 const char *xs_daemon_socket_ro(void);
 
-/* Simple write function: loops for you. */
-bool xs_write_all(int fd, const void *data, unsigned int len);
-
 /* Convert strings to permissions.  False if a problem. */
 bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
 			 const char *strings);
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 76ffb1be45..bb93246bfb 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -311,6 +311,26 @@ struct xs_handle *xs_domain_open(void)
 	return xs_open(0);
 }
 
+static const char *xs_domain_dev(void)
+{
+	char *s = getenv("XENSTORED_PATH");
+	if (s)
+		return s;
+#if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
+	return "/dev/xen/xenbus";
+#elif defined(__linux__)
+	if (access("/dev/xen/xenbus", F_OK) == 0)
+		return "/dev/xen/xenbus";
+	return "/proc/xen/xenbus";
+#elif defined(__NetBSD__)
+	return "/kern/xen/xenbus";
+#elif defined(__FreeBSD__)
+	return "/dev/xen/xenstore";
+#else
+	return "/dev/xen/xenbus";
+#endif
+}
+
 struct xs_handle *xs_open(unsigned long flags)
 {
 	struct xs_handle *xsh = NULL;
@@ -431,6 +451,24 @@ out_false:
 #ifdef XSTEST
 #define read_all read_all_choice
 #define xs_write_all write_all_choice
+#else
+/* Simple routine for writing to sockets, etc. */
+bool xs_write_all(int fd, const void *data, unsigned int len)
+{
+	while (len) {
+		int done;
+
+		done = write(fd, data, len);
+		if (done < 0 && errno == EINTR)
+			continue;
+		if (done <= 0)
+			return false;
+		data += done;
+		len -= done;
+	}
+
+	return true;
+}
 #endif
 
 static int get_error(const char *errorstring)
@@ -1328,6 +1366,11 @@ error:
 	return ret;
 }
 
+const char *xs_daemon_socket_ro(void)
+{
+	return xs_daemon_socket();
+}
+
 #ifdef USE_PTHREAD
 static void *read_thread(void *arg)
 {
diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 0628ba275e..8ff8abf12a 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include <assert.h>
 #include <err.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -40,12 +41,140 @@ enum mode {
     MODE_watch,
 };
 
+/* Sanitising (quoting) possibly-binary strings. */
+struct expanding_buffer {
+    char *buf;
+    int avail;
+};
+
 static char *output_buf = NULL;
 static int output_pos = 0;
 static struct expanding_buffer ebuf;
 
 static int output_size = 0;
 
+/* Ensure that given expanding buffer has at least min_avail characters. */
+static char *expanding_buffer_ensure(struct expanding_buffer *ebuf,
+                                     int min_avail)
+{
+    int want;
+    char *got;
+
+    if ( ebuf->avail >= min_avail )
+        return ebuf->buf;
+
+    if ( min_avail >= INT_MAX/3 )
+        return 0;
+
+    want = ebuf->avail + min_avail + 10;
+    got = realloc(ebuf->buf, want);
+    if ( !got )
+        return 0;
+
+    ebuf->buf = got;
+    ebuf->avail = want;
+    return ebuf->buf;
+}
+
+/* sanitise_value() may return NULL if malloc fails. */
+static char *sanitise_value(struct expanding_buffer *ebuf,
+                            const char *val, unsigned len)
+{
+    int used, remain, c;
+    unsigned char *ip;
+
+#define ADD(c) (ebuf->buf[used++] = (c))
+#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
+
+    assert(len < INT_MAX/5);
+
+    ip = (unsigned char *)val;
+    used = 0;
+    remain = len;
+
+    if ( !expanding_buffer_ensure(ebuf, remain + 1) )
+        return NULL;
+
+    while ( remain-- > 0 )
+    {
+        c= *ip++;
+
+        if ( c >= ' ' && c <= '~' && c != '\\' )
+        {
+            ADD(c);
+            continue;
+        }
+
+        if ( !expanding_buffer_ensure(ebuf, used + remain + 5) )
+            /* for "<used>\\nnn<remain>\0" */
+            return 0;
+
+        ADD('\\');
+        switch (c)
+        {
+        case '\t':  ADD('t');   break;
+        case '\n':  ADD('n');   break;
+        case '\r':  ADD('r');   break;
+        case '\\':  ADD('\\');  break;
+        default:
+            if ( c < 010 ) ADDF("%03o", c);
+            else           ADDF("x%02x", c);
+        }
+    }
+
+    ADD(0);
+    assert(used <= ebuf->avail);
+    return ebuf->buf;
+
+#undef ADD
+#undef ADDF
+}
+
+/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
+static void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
+{
+    const char *ip;
+    char *op;
+    unsigned c;
+    int n;
+
+    for ( ip = in, op = out; (c = *ip++); *op++ = c )
+    {
+        if ( c == '\\' )
+        {
+            c = *ip++;
+
+#define GETF(f) do                   \
+{                                    \
+     n = 0;                          \
+     sscanf(ip, f "%n", &c, &n);     \
+     ip += n;                        \
+} while ( 0 )
+
+            switch ( c )
+            {
+            case 't':           c= '\t';           break;
+            case 'n':           c= '\n';           break;
+            case 'r':           c= '\r';           break;
+            case '\\':          c= '\\';           break;
+            case 'x':           GETF("%2x");       break;
+            case '0': case '4':
+            case '1': case '5':
+            case '2': case '6':
+            case '3': case '7': --ip; GETF("%3o"); break;
+            case 0:             --ip;              break;
+            default:;
+            }
+#undef GETF
+        }
+    }
+
+    *op = 0;
+
+    if ( out_len_r )
+        *out_len_r = op - out;
+}
+
 /* make sure there is at least 'len' more space in output_buf */
 static void expand_buffer(size_t len)
 {
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index ed80924ee4..6ef800ff64 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -560,7 +560,7 @@ static FILE *lu_dump_open(const void *ctx)
 	char *filename;
 	int fd;
 
-	filename = talloc_asprintf(ctx, "%s/state_dump", xs_daemon_rootdir());
+	filename = talloc_asprintf(ctx, "%s/state_dump", xs_daemon_rundir());
 	if (!filename)
 		return NULL;
 
@@ -583,7 +583,7 @@ static void lu_get_dump_state(struct lu_dump_state *state)
 	state->size = 0;
 
 	state->filename = talloc_asprintf(NULL, "%s/state_dump",
-					  xs_daemon_rootdir());
+					  xs_daemon_rundir());
 	if (!state->filename)
 		barf("Allocation failure");
 
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 07fbac55ac..a78cbf1116 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2888,10 +2888,9 @@ int main(int argc, char *argv[])
 
 	reopen_log();
 
-	/* make sure xenstored directories exist */
+	/* Make sure xenstored directory exists. */
 	/* Errors ignored here, will be reported when we open files */
 	mkdir(xs_daemon_rundir(), 0755);
-	mkdir(xs_daemon_rootdir(), 0755);
 
 	if (dofork) {
 		openlog("xenstored", 0, LOG_DAEMON);
diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index b9941c567c..826fa7b5a8 100644
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -32,11 +32,6 @@ const char *xs_daemon_rundir(void)
 	return (s ? s : XEN_RUN_STORED);
 }
 
-const char *xs_daemon_rootdir(void)
-{
-	return xs_daemon_rundir();
-}
-
 static const char *xs_daemon_path(void)
 {
 	static char buf[PATH_MAX];
@@ -49,61 +44,11 @@ static const char *xs_daemon_path(void)
 	return buf;
 }
 
-const char *xs_daemon_tdb(void)
-{
-	static char buf[PATH_MAX];
-	snprintf(buf, sizeof(buf), "%s/tdb", xs_daemon_rootdir());
-	return buf;
-}
-
 const char *xs_daemon_socket(void)
 {
 	return xs_daemon_path();
 }
 
-const char *xs_daemon_socket_ro(void)
-{
-	return xs_daemon_path();
-}
-
-const char *xs_domain_dev(void)
-{
-	char *s = getenv("XENSTORED_PATH");
-	if (s)
-		return s;
-#if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
-	return "/dev/xen/xenbus";
-#elif defined(__linux__)
-	if (access("/dev/xen/xenbus", F_OK) == 0)
-		return "/dev/xen/xenbus";
-	return "/proc/xen/xenbus";
-#elif defined(__NetBSD__)
-	return "/kern/xen/xenbus";
-#elif defined(__FreeBSD__)
-	return "/dev/xen/xenstore";
-#else
-	return "/dev/xen/xenbus";
-#endif
-}
-
-/* Simple routines for writing to sockets, etc. */
-bool xs_write_all(int fd, const void *data, unsigned int len)
-{
-	while (len) {
-		int done;
-
-		done = write(fd, data, len);
-		if (done < 0 && errno == EINTR)
-			continue;
-		if (done <= 0)
-			return false;
-		data += done;
-		len -= done;
-	}
-
-	return true;
-}
-
 /* Convert strings to permissions.  False if a problem. */
 bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
 			 const char *strings)
@@ -179,114 +124,3 @@ unsigned int xs_count_strings(const char *strings, unsigned int len)
 
 	return num;
 }
-
-char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
-{
-	int want;
-	char *got;
-
-	if (ebuf->avail >= min_avail)
-		return ebuf->buf;
-
-	if (min_avail >= INT_MAX/3)
-		return 0;
-
-	want = ebuf->avail + min_avail + 10;
-	got = realloc(ebuf->buf, want);
-	if (!got)
-		return 0;
-
-	ebuf->buf = got;
-	ebuf->avail = want;
-	return ebuf->buf;
-}
-
-char *sanitise_value(struct expanding_buffer *ebuf,
-		     const char *val, unsigned len)
-{
-	int used, remain, c;
-	unsigned char *ip;
-
-#define ADD(c) (ebuf->buf[used++] = (c))
-#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
-
-	assert(len < INT_MAX/5);
-
-	ip = (unsigned char *)val;
-	used = 0;
-	remain = len;
-
-	if (!expanding_buffer_ensure(ebuf, remain + 1))
-		return NULL;
-
-	while (remain-- > 0) {
-		c= *ip++;
-
-		if (c >= ' ' && c <= '~' && c != '\\') {
-			ADD(c);
-			continue;
-		}
-
-		if (!expanding_buffer_ensure(ebuf, used + remain + 5))
-			/* for "<used>\\nnn<remain>\0" */
-			return 0;
-
-		ADD('\\');
-		switch (c) {
-		case '\t':  ADD('t');   break;
-		case '\n':  ADD('n');   break;
-		case '\r':  ADD('r');   break;
-		case '\\':  ADD('\\');  break;
-		default:
-			if (c < 010) ADDF("%03o", c);
-			else         ADDF("x%02x", c);
-		}
-	}
-
-	ADD(0);
-	assert(used <= ebuf->avail);
-	return ebuf->buf;
-
-#undef ADD
-#undef ADDF
-}
-
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
-{
-	const char *ip;
-	char *op;
-	unsigned c;
-	int n;
-
-	for (ip = in, op = out; (c = *ip++); *op++ = c) {
-		if (c == '\\') {
-			c = *ip++;
-
-#define GETF(f) do {					\
-			n = 0;				\
-			sscanf(ip, f "%n", &c, &n);	\
-			ip += n;			\
-		} while (0)
-
-			switch (c) {
-			case 't':		c= '\t';		break;
-			case 'n':		c= '\n';		break;
-			case 'r':		c= '\r';		break;
-			case '\\':		c= '\\';		break;
-			case 'x':		GETF("%2x");		break;
-			case '0': case '4':
-			case '1': case '5':
-			case '2': case '6':
-			case '3': case '7':	--ip; GETF("%3o");	break;
-			case 0:			--ip;			break;
-			default:;
-			}
-#undef GETF
-		}
-	}
-
-	*op = 0;
-
-	if (out_len_r)
-		*out_len_r = op - out;
-}
diff --git a/tools/xenstore/xs_lib.h b/tools/xenstore/xs_lib.h
index efa05997d6..5b9874d741 100644
--- a/tools/xenstore/xs_lib.h
+++ b/tools/xenstore/xs_lib.h
@@ -21,10 +21,6 @@
 
 #include "xenstore_lib.h"
 
-const char *xs_daemon_rootdir(void);
-const char *xs_domain_dev(void);
-const char *xs_daemon_tdb(void);
-
 /* Convert permissions to a string (up to len MAX_STRLEN(unsigned int)+1). */
 bool xs_perm_to_string(const struct xs_permissions *perm,
 		       char *buffer, size_t buf_len);
@@ -32,19 +28,4 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
 /* Given a string and a length, count how many strings (nul terms). */
 unsigned int xs_count_strings(const char *strings, unsigned int len);
 
-/* Sanitising (quoting) possibly-binary strings. */
-struct expanding_buffer {
-	char *buf;
-	int avail;
-};
-
-/* Ensure that given expanding buffer has at least min_avail characters. */
-char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
-
-/* sanitise_value() may return NULL if malloc fails. */
-char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
-
-/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
-
 #endif /* XS_LIB_H */
-- 
2.35.3

[PATCH v3 13/16] tools/xenstore: replace xs_lib.c with a header

[Juergen Gross]

Instead of including the same small C source in multiple binaries from
2 source directories, use a header file with inline functions as a
replacement.

As some of the functions are exported by libxenstore, rename the inline
functions from xs_*() do xenstore_*() and add xs_*() wrappers to
libxenstore.

With that no sources required to build libxenstore are left in
tools/xenstore, so the file COPYING can be removed now.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 MAINTAINERS                               |   1 +
 tools/include/xen-tools/xenstore-common.h | 128 ++++++
 tools/libs/store/Makefile                 |   4 -
 tools/libs/store/xs.c                     |  28 +-
 tools/xenstore/COPYING                    | 514 ----------------------
 tools/xenstore/Makefile                   |   2 +-
 tools/xenstore/Makefile.common            |   2 +-
 tools/xenstore/xenstore_client.c          |   4 +-
 tools/xenstore/xenstored_control.c        |   9 +-
 tools/xenstore/xenstored_core.c           |  15 +-
 tools/xenstore/xs_lib.c                   | 126 ------
 tools/xenstore/xs_lib.h                   |  31 --
 12 files changed, 168 insertions(+), 696 deletions(-)
 create mode 100644 tools/include/xen-tools/xenstore-common.h
 delete mode 100644 tools/xenstore/COPYING
 delete mode 100644 tools/xenstore/xs_lib.c
 delete mode 100644 tools/xenstore/xs_lib.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f2f1881b32..751703373d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -657,6 +657,7 @@ F:	tools/helpers/init-xenstore-domain.c
 F:	tools/include/xenstore-compat/
 F:	tools/include/xenstore.h
 F:	tools/include/xenstore_lib.h
+F:	tools/include/xen-tools/xenstore-common.h
 F:	tools/libs/store/
 F:	tools/xenstore/
 
diff --git a/tools/include/xen-tools/xenstore-common.h b/tools/include/xen-tools/xenstore-common.h
new file mode 100644
index 0000000000..8cb097a8fc
--- /dev/null
+++ b/tools/include/xen-tools/xenstore-common.h
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/* Common functions for libxenstore, xenstored and xenstore-clients. */
+
+#ifndef __XEN_TOOLS_XENSTORE_COMMON__
+#define __XEN_TOOLS_XENSTORE_COMMON__
+
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <xenstore_lib.h>
+
+static inline const char *xenstore_daemon_rundir(void)
+{
+    char *s = getenv("XENSTORED_RUNDIR");
+
+    return s ? s : XEN_RUN_STORED;
+}
+
+static inline const char *xenstore_daemon_path(void)
+{
+    static char buf[PATH_MAX];
+    char *s = getenv("XENSTORED_PATH");
+
+    if ( s )
+        return s;
+
+    if ( snprintf(buf, sizeof(buf), "%s/socket", xenstore_daemon_rundir()) >=
+         PATH_MAX )
+        return NULL;
+
+    return buf;
+}
+
+/* Convert strings to permissions.  False if a problem. */
+static inline bool xenstore_strings_to_perms(struct xs_permissions *perms,
+                                             unsigned int num,
+                                             const char *strings)
+{
+    const char *p;
+    char *end;
+    unsigned int i;
+
+    for ( p = strings, i = 0; i < num; i++ )
+    {
+        /* "r", "w", or "b" for both. */
+        switch ( *p )
+        {
+        case 'r':
+            perms[i].perms = XS_PERM_READ;
+            break;
+
+        case 'w':
+            perms[i].perms = XS_PERM_WRITE;
+            break;
+
+        case 'b':
+            perms[i].perms = XS_PERM_READ|XS_PERM_WRITE;
+            break;
+
+        case 'n':
+            perms[i].perms = XS_PERM_NONE;
+            break;
+
+        default:
+            errno = EINVAL;
+            return false;
+        }
+
+        p++;
+        perms[i].id = strtol(p, &end, 0);
+        if ( *end || !*p )
+        {
+            errno = EINVAL;
+            return false;
+        }
+
+        p = end + 1;
+    }
+
+    return true;
+}
+
+/* Convert permissions to a string (up to len MAX_STRLEN(unsigned int)+1). */
+static inline bool xenstore_perm_to_string(const struct xs_permissions *perm,
+                                           char *buffer, size_t buf_len)
+{
+    switch ( (int)perm->perms & ~XS_PERM_IGNORE )
+    {
+    case XS_PERM_WRITE:
+        *buffer = 'w';
+        break;
+
+    case XS_PERM_READ:
+        *buffer = 'r';
+        break;
+
+    case XS_PERM_READ|XS_PERM_WRITE:
+        *buffer = 'b';
+        break;
+
+    case XS_PERM_NONE:
+        *buffer = 'n';
+        break;
+
+    default:
+        errno = EINVAL;
+        return false;
+    }
+
+    snprintf(buffer + 1, buf_len - 1, "%i", (int)perm->id);
+
+    return true;
+}
+
+/* Given a string and a length, count how many strings (nul terms). */
+static inline unsigned int xenstore_count_strings(const char *strings,
+                                                  unsigned int len)
+{
+    unsigned int num;
+    const char *p;
+
+    for ( p = strings, num = 0; p < strings + len; p++ )
+        if ( *p == '\0' )
+            num++;
+
+    return num;
+}
+#endif /* __XEN_TOOLS_XENSTORE_COMMON__ */
diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
index daed9d148f..c1a1508713 100644
--- a/tools/libs/store/Makefile
+++ b/tools/libs/store/Makefile
@@ -9,7 +9,6 @@ ifeq ($(CONFIG_Linux),y)
 LDLIBS += -ldl
 endif
 
-OBJS-y   += xs_lib.o
 OBJS-y   += xs.o
 
 LIBHEADER = xenstore.h xenstore_lib.h
@@ -21,9 +20,6 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
 CFLAGS += $(CFLAGS_libxentoolcore)
 CFLAGS += -DXEN_RUN_STORED="\"$(XEN_RUN_STORED)\""
 
-vpath xs_lib.c $(XEN_ROOT)/tools/xenstore
-CFLAGS += -iquote $(XEN_ROOT)/tools/xenstore
-
 xs.opic: CFLAGS += -DUSE_PTHREAD
 ifeq ($(CONFIG_Linux),y)
 xs.opic: CFLAGS += -DUSE_DLSYM
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index bb93246bfb..140b9a2839 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -34,8 +34,8 @@
 #include <stdint.h>
 #include <errno.h>
 #include <xen-tools/common-macros.h>
+#include <xen-tools/xenstore-common.h>
 #include "xenstore.h"
-#include "xs_lib.h"
 
 #include <xentoolcore_internal.h>
 #include <xen_list.h>
@@ -627,7 +627,7 @@ static char **xs_directory_common(char *strings, unsigned int len,
 	char *p, **ret;
 
 	/* Count the strings. */
-	*num = xs_count_strings(strings, len);
+	*num = xenstore_count_strings(strings, len);
 
 	/* Transfer to one big alloc for easy freeing. */
 	ret = malloc(*num * sizeof(char *) + len);
@@ -775,7 +775,7 @@ struct xs_permissions *xs_get_permissions(struct xs_handle *h,
 		return NULL;
 
 	/* Count the strings: each one perms then domid. */
-	*num = xs_count_strings(strings, len);
+	*num = xenstore_count_strings(strings, len);
 
 	/* Transfer to one big alloc for easy freeing. */
 	ret = malloc(*num * sizeof(struct xs_permissions));
@@ -784,7 +784,7 @@ struct xs_permissions *xs_get_permissions(struct xs_handle *h,
 		return NULL;
 	}
 
-	if (!xs_strings_to_perms(ret, *num, strings)) {
+	if (!xenstore_strings_to_perms(ret, *num, strings)) {
 		free_no_errno(ret);
 		ret = NULL;
 	}
@@ -811,7 +811,7 @@ bool xs_set_permissions(struct xs_handle *h,
 	for (i = 0; i < num_perms; i++) {
 		char buffer[MAX_STRLEN(unsigned int)+1];
 
-		if (!xs_perm_to_string(&perms[i], buffer, sizeof(buffer)))
+		if (!xenstore_perm_to_string(&perms[i], buffer, sizeof(buffer)))
 			goto unwind;
 
 		iov[i+1].iov_base = strdup(buffer);
@@ -977,7 +977,7 @@ static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
 	assert(msg->hdr.type == XS_WATCH_EVENT);
 
 	strings     = msg->body;
-	num_strings = xs_count_strings(strings, msg->hdr.len);
+	num_strings = xenstore_count_strings(strings, msg->hdr.len);
 
 	ret = malloc(sizeof(char*) * num_strings + msg->hdr.len);
 	if (!ret) {
@@ -1366,11 +1366,27 @@ error:
 	return ret;
 }
 
+const char *xs_daemon_socket(void)
+{
+	return xenstore_daemon_path();
+}
+
 const char *xs_daemon_socket_ro(void)
 {
 	return xs_daemon_socket();
 }
 
+const char *xs_daemon_rundir(void)
+{
+	return xenstore_daemon_rundir();
+}
+
+bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
+			 const char *strings)
+{
+	return xenstore_strings_to_perms(perms, num, strings);
+}
+
 #ifdef USE_PTHREAD
 static void *read_thread(void *arg)
 {
diff --git a/tools/xenstore/COPYING b/tools/xenstore/COPYING
deleted file mode 100644
index c764b2e329..0000000000
--- a/tools/xenstore/COPYING
+++ /dev/null
@@ -1,514 +0,0 @@
-This license (LGPL) applies to the xenstore library which interfaces
-with the xenstore daemon (as stated in xs.c, xenstore.h, xs_lib.c and
-xenstore_lib.h).  The remaining files in the directory are licensed as
-stated in the comments (as of this writing, GPL, see ../../COPYING).
-
-
-                  GNU LESSER GENERAL PUBLIC LICENSE
-                       Version 2.1, February 1999
-
- Copyright (C) 1991, 1999 Free Software Foundation, Inc.
-	51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- Everyone is permitted to copy and distribute verbatim copies
- of this license document, but changing it is not allowed.
-
-[This is the first released version of the Lesser GPL.  It also counts
- as the successor of the GNU Library Public License, version 2, hence
- the version number 2.1.]
-
-                            Preamble
-
-  The licenses for most software are designed to take away your
-freedom to share and change it.  By contrast, the GNU General Public
-Licenses are intended to guarantee your freedom to share and change
-free software--to make sure the software is free for all its users.
-
-  This license, the Lesser General Public License, applies to some
-specially designated software packages--typically libraries--of the
-Free Software Foundation and other authors who decide to use it.  You
-can use it too, but we suggest you first think carefully about whether
-this license or the ordinary General Public License is the better
-strategy to use in any particular case, based on the explanations
-below.
-
-  When we speak of free software, we are referring to freedom of use,
-not price.  Our General Public Licenses are designed to make sure that
-you have the freedom to distribute copies of free software (and charge
-for this service if you wish); that you receive source code or can get
-it if you want it; that you can change the software and use pieces of
-it in new free programs; and that you are informed that you can do
-these things.
-
-  To protect your rights, we need to make restrictions that forbid
-distributors to deny you these rights or to ask you to surrender these
-rights.  These restrictions translate to certain responsibilities for
-you if you distribute copies of the library or if you modify it.
-
-  For example, if you distribute copies of the library, whether gratis
-or for a fee, you must give the recipients all the rights that we gave
-you.  You must make sure that they, too, receive or can get the source
-code.  If you link other code with the library, you must provide
-complete object files to the recipients, so that they can relink them
-with the library after making changes to the library and recompiling
-it.  And you must show them these terms so they know their rights.
-
-  We protect your rights with a two-step method: (1) we copyright the
-library, and (2) we offer you this license, which gives you legal
-permission to copy, distribute and/or modify the library.
-
-  To protect each distributor, we want to make it very clear that
-there is no warranty for the free library.  Also, if the library is
-modified by someone else and passed on, the recipients should know
-that what they have is not the original version, so that the original
-author's reputation will not be affected by problems that might be
-introduced by others.
-\f
-  Finally, software patents pose a constant threat to the existence of
-any free program.  We wish to make sure that a company cannot
-effectively restrict the users of a free program by obtaining a
-restrictive license from a patent holder.  Therefore, we insist that
-any patent license obtained for a version of the library must be
-consistent with the full freedom of use specified in this license.
-
-  Most GNU software, including some libraries, is covered by the
-ordinary GNU General Public License.  This license, the GNU Lesser
-General Public License, applies to certain designated libraries, and
-is quite different from the ordinary General Public License.  We use
-this license for certain libraries in order to permit linking those
-libraries into non-free programs.
-
-  When a program is linked with a library, whether statically or using
-a shared library, the combination of the two is legally speaking a
-combined work, a derivative of the original library.  The ordinary
-General Public License therefore permits such linking only if the
-entire combination fits its criteria of freedom.  The Lesser General
-Public License permits more lax criteria for linking other code with
-the library.
-
-  We call this license the "Lesser" General Public License because it
-does Less to protect the user's freedom than the ordinary General
-Public License.  It also provides other free software developers Less
-of an advantage over competing non-free programs.  These disadvantages
-are the reason we use the ordinary General Public License for many
-libraries.  However, the Lesser license provides advantages in certain
-special circumstances.
-
-  For example, on rare occasions, there may be a special need to
-encourage the widest possible use of a certain library, so that it
-becomes a de-facto standard.  To achieve this, non-free programs must
-be allowed to use the library.  A more frequent case is that a free
-library does the same job as widely used non-free libraries.  In this
-case, there is little to gain by limiting the free library to free
-software only, so we use the Lesser General Public License.
-
-  In other cases, permission to use a particular library in non-free
-programs enables a greater number of people to use a large body of
-free software.  For example, permission to use the GNU C Library in
-non-free programs enables many more people to use the whole GNU
-operating system, as well as its variant, the GNU/Linux operating
-system.
-
-  Although the Lesser General Public License is Less protective of the
-users' freedom, it does ensure that the user of a program that is
-linked with the Library has the freedom and the wherewithal to run
-that program using a modified version of the Library.
-
-  The precise terms and conditions for copying, distribution and
-modification follow.  Pay close attention to the difference between a
-"work based on the library" and a "work that uses the library".  The
-former contains code derived from the library, whereas the latter must
-be combined with the library in order to run.
-\f
-                  GNU LESSER GENERAL PUBLIC LICENSE
-   TERMS AND CONDITIONS FOR COPYING, DISTRIBUTION AND MODIFICATION
-
-  0. This License Agreement applies to any software library or other
-program which contains a notice placed by the copyright holder or
-other authorized party saying it may be distributed under the terms of
-this Lesser General Public License (also called "this License").
-Each licensee is addressed as "you".
-
-  A "library" means a collection of software functions and/or data
-prepared so as to be conveniently linked with application programs
-(which use some of those functions and data) to form executables.
-
-  The "Library", below, refers to any such software library or work
-which has been distributed under these terms.  A "work based on the
-Library" means either the Library or any derivative work under
-copyright law: that is to say, a work containing the Library or a
-portion of it, either verbatim or with modifications and/or translated
-straightforwardly into another language.  (Hereinafter, translation is
-included without limitation in the term "modification".)
-
-  "Source code" for a work means the preferred form of the work for
-making modifications to it.  For a library, complete source code means
-all the source code for all modules it contains, plus any associated
-interface definition files, plus the scripts used to control
-compilation and installation of the library.
-
-  Activities other than copying, distribution and modification are not
-covered by this License; they are outside its scope.  The act of
-running a program using the Library is not restricted, and output from
-such a program is covered only if its contents constitute a work based
-on the Library (independent of the use of the Library in a tool for
-writing it).  Whether that is true depends on what the Library does
-and what the program that uses the Library does.
-
-  1. You may copy and distribute verbatim copies of the Library's
-complete source code as you receive it, in any medium, provided that
-you conspicuously and appropriately publish on each copy an
-appropriate copyright notice and disclaimer of warranty; keep intact
-all the notices that refer to this License and to the absence of any
-warranty; and distribute a copy of this License along with the
-Library.
-
-  You may charge a fee for the physical act of transferring a copy,
-and you may at your option offer warranty protection in exchange for a
-fee.
-\f
-  2. You may modify your copy or copies of the Library or any portion
-of it, thus forming a work based on the Library, and copy and
-distribute such modifications or work under the terms of Section 1
-above, provided that you also meet all of these conditions:
-
-    a) The modified work must itself be a software library.
-
-    b) You must cause the files modified to carry prominent notices
-    stating that you changed the files and the date of any change.
-
-    c) You must cause the whole of the work to be licensed at no
-    charge to all third parties under the terms of this License.
-
-    d) If a facility in the modified Library refers to a function or a
-    table of data to be supplied by an application program that uses
-    the facility, other than as an argument passed when the facility
-    is invoked, then you must make a good faith effort to ensure that,
-    in the event an application does not supply such function or
-    table, the facility still operates, and performs whatever part of
-    its purpose remains meaningful.
-
-    (For example, a function in a library to compute square roots has
-    a purpose that is entirely well-defined independent of the
-    application.  Therefore, Subsection 2d requires that any
-    application-supplied function or table used by this function must
-    be optional: if the application does not supply it, the square
-    root function must still compute square roots.)
-
-These requirements apply to the modified work as a whole.  If
-identifiable sections of that work are not derived from the Library,
-and can be reasonably considered independent and separate works in
-themselves, then this License, and its terms, do not apply to those
-sections when you distribute them as separate works.  But when you
-distribute the same sections as part of a whole which is a work based
-on the Library, the distribution of the whole must be on the terms of
-this License, whose permissions for other licensees extend to the
-entire whole, and thus to each and every part regardless of who wrote
-it.
-
-Thus, it is not the intent of this section to claim rights or contest
-your rights to work written entirely by you; rather, the intent is to
-exercise the right to control the distribution of derivative or
-collective works based on the Library.
-
-In addition, mere aggregation of another work not based on the Library
-with the Library (or with a work based on the Library) on a volume of
-a storage or distribution medium does not bring the other work under
-the scope of this License.
-
-  3. You may opt to apply the terms of the ordinary GNU General Public
-License instead of this License to a given copy of the Library.  To do
-this, you must alter all the notices that refer to this License, so
-that they refer to the ordinary GNU General Public License, version 2,
-instead of to this License.  (If a newer version than version 2 of the
-ordinary GNU General Public License has appeared, then you can specify
-that version instead if you wish.)  Do not make any other change in
-these notices.
-\f
-  Once this change is made in a given copy, it is irreversible for
-that copy, so the ordinary GNU General Public License applies to all
-subsequent copies and derivative works made from that copy.
-
-  This option is useful when you wish to copy part of the code of
-the Library into a program that is not a library.
-
-  4. You may copy and distribute the Library (or a portion or
-derivative of it, under Section 2) in object code or executable form
-under the terms of Sections 1 and 2 above provided that you accompany
-it with the complete corresponding machine-readable source code, which
-must be distributed under the terms of Sections 1 and 2 above on a
-medium customarily used for software interchange.
-
-  If distribution of object code is made by offering access to copy
-from a designated place, then offering equivalent access to copy the
-source code from the same place satisfies the requirement to
-distribute the source code, even though third parties are not
-compelled to copy the source along with the object code.
-
-  5. A program that contains no derivative of any portion of the
-Library, but is designed to work with the Library by being compiled or
-linked with it, is called a "work that uses the Library".  Such a
-work, in isolation, is not a derivative work of the Library, and
-therefore falls outside the scope of this License.
-
-  However, linking a "work that uses the Library" with the Library
-creates an executable that is a derivative of the Library (because it
-contains portions of the Library), rather than a "work that uses the
-library".  The executable is therefore covered by this License.
-Section 6 states terms for distribution of such executables.
-
-  When a "work that uses the Library" uses material from a header file
-that is part of the Library, the object code for the work may be a
-derivative work of the Library even though the source code is not.
-Whether this is true is especially significant if the work can be
-linked without the Library, or if the work is itself a library.  The
-threshold for this to be true is not precisely defined by law.
-
-  If such an object file uses only numerical parameters, data
-structure layouts and accessors, and small macros and small inline
-functions (ten lines or less in length), then the use of the object
-file is unrestricted, regardless of whether it is legally a derivative
-work.  (Executables containing this object code plus portions of the
-Library will still fall under Section 6.)
-
-  Otherwise, if the work is a derivative of the Library, you may
-distribute the object code for the work under the terms of Section 6.
-Any executables containing that work also fall under Section 6,
-whether or not they are linked directly with the Library itself.
-\f
-  6. As an exception to the Sections above, you may also combine or
-link a "work that uses the Library" with the Library to produce a
-work containing portions of the Library, and distribute that work
-under terms of your choice, provided that the terms permit
-modification of the work for the customer's own use and reverse
-engineering for debugging such modifications.
-
-  You must give prominent notice with each copy of the work that the
-Library is used in it and that the Library and its use are covered by
-this License.  You must supply a copy of this License.  If the work
-during execution displays copyright notices, you must include the
-copyright notice for the Library among them, as well as a reference
-directing the user to the copy of this License.  Also, you must do one
-of these things:
-
-    a) Accompany the work with the complete corresponding
-    machine-readable source code for the Library including whatever
-    changes were used in the work (which must be distributed under
-    Sections 1 and 2 above); and, if the work is an executable linked
-    with the Library, with the complete machine-readable "work that
-    uses the Library", as object code and/or source code, so that the
-    user can modify the Library and then relink to produce a modified
-    executable containing the modified Library.  (It is understood
-    that the user who changes the contents of definitions files in the
-    Library will not necessarily be able to recompile the application
-    to use the modified definitions.)
-
-    b) Use a suitable shared library mechanism for linking with the
-    Library.  A suitable mechanism is one that (1) uses at run time a
-    copy of the library already present on the user's computer system,
-    rather than copying library functions into the executable, and (2)
-    will operate properly with a modified version of the library, if
-    the user installs one, as long as the modified version is
-    interface-compatible with the version that the work was made with.
-
-    c) Accompany the work with a written offer, valid for at least
-    three years, to give the same user the materials specified in
-    Subsection 6a, above, for a charge no more than the cost of
-    performing this distribution.
-
-    d) If distribution of the work is made by offering access to copy
-    from a designated place, offer equivalent access to copy the above
-    specified materials from the same place.
-
-    e) Verify that the user has already received a copy of these
-    materials or that you have already sent this user a copy.
-
-  For an executable, the required form of the "work that uses the
-Library" must include any data and utility programs needed for
-reproducing the executable from it.  However, as a special exception,
-the materials to be distributed need not include anything that is
-normally distributed (in either source or binary form) with the major
-components (compiler, kernel, and so on) of the operating system on
-which the executable runs, unless that component itself accompanies
-the executable.
-
-  It may happen that this requirement contradicts the license
-restrictions of other proprietary libraries that do not normally
-accompany the operating system.  Such a contradiction means you cannot
-use both them and the Library together in an executable that you
-distribute.
-\f
-  7. You may place library facilities that are a work based on the
-Library side-by-side in a single library together with other library
-facilities not covered by this License, and distribute such a combined
-library, provided that the separate distribution of the work based on
-the Library and of the other library facilities is otherwise
-permitted, and provided that you do these two things:
-
-    a) Accompany the combined library with a copy of the same work
-    based on the Library, uncombined with any other library
-    facilities.  This must be distributed under the terms of the
-    Sections above.
-
-    b) Give prominent notice with the combined library of the fact
-    that part of it is a work based on the Library, and explaining
-    where to find the accompanying uncombined form of the same work.
-
-  8. You may not copy, modify, sublicense, link with, or distribute
-the Library except as expressly provided under this License.  Any
-attempt otherwise to copy, modify, sublicense, link with, or
-distribute the Library is void, and will automatically terminate your
-rights under this License.  However, parties who have received copies,
-or rights, from you under this License will not have their licenses
-terminated so long as such parties remain in full compliance.
-
-  9. You are not required to accept this License, since you have not
-signed it.  However, nothing else grants you permission to modify or
-distribute the Library or its derivative works.  These actions are
-prohibited by law if you do not accept this License.  Therefore, by
-modifying or distributing the Library (or any work based on the
-Library), you indicate your acceptance of this License to do so, and
-all its terms and conditions for copying, distributing or modifying
-the Library or works based on it.
-
-  10. Each time you redistribute the Library (or any work based on the
-Library), the recipient automatically receives a license from the
-original licensor to copy, distribute, link with or modify the Library
-subject to these terms and conditions.  You may not impose any further
-restrictions on the recipients' exercise of the rights granted herein.
-You are not responsible for enforcing compliance by third parties with
-this License.
-\f
-  11. If, as a consequence of a court judgment or allegation of patent
-infringement or for any other reason (not limited to patent issues),
-conditions are imposed on you (whether by court order, agreement or
-otherwise) that contradict the conditions of this License, they do not
-excuse you from the conditions of this License.  If you cannot
-distribute so as to satisfy simultaneously your obligations under this
-License and any other pertinent obligations, then as a consequence you
-may not distribute the Library at all.  For example, if a patent
-license would not permit royalty-free redistribution of the Library by
-all those who receive copies directly or indirectly through you, then
-the only way you could satisfy both it and this License would be to
-refrain entirely from distribution of the Library.
-
-If any portion of this section is held invalid or unenforceable under
-any particular circumstance, the balance of the section is intended to
-apply, and the section as a whole is intended to apply in other
-circumstances.
-
-It is not the purpose of this section to induce you to infringe any
-patents or other property right claims or to contest validity of any
-such claims; this section has the sole purpose of protecting the
-integrity of the free software distribution system which is
-implemented by public license practices.  Many people have made
-generous contributions to the wide range of software distributed
-through that system in reliance on consistent application of that
-system; it is up to the author/donor to decide if he or she is willing
-to distribute software through any other system and a licensee cannot
-impose that choice.
-
-This section is intended to make thoroughly clear what is believed to
-be a consequence of the rest of this License.
-
-  12. If the distribution and/or use of the Library is restricted in
-certain countries either by patents or by copyrighted interfaces, the
-original copyright holder who places the Library under this License
-may add an explicit geographical distribution limitation excluding those
-countries, so that distribution is permitted only in or among
-countries not thus excluded.  In such case, this License incorporates
-the limitation as if written in the body of this License.
-
-  13. The Free Software Foundation may publish revised and/or new
-versions of the Lesser General Public License from time to time.
-Such new versions will be similar in spirit to the present version,
-but may differ in detail to address new problems or concerns.
-
-Each version is given a distinguishing version number.  If the Library
-specifies a version number of this License which applies to it and
-"any later version", you have the option of following the terms and
-conditions either of that version or of any later version published by
-the Free Software Foundation.  If the Library does not specify a
-license version number, you may choose any version ever published by
-the Free Software Foundation.
-\f
-  14. If you wish to incorporate parts of the Library into other free
-programs whose distribution conditions are incompatible with these,
-write to the author to ask for permission.  For software which is
-copyrighted by the Free Software Foundation, write to the Free
-Software Foundation; we sometimes make exceptions for this.  Our
-decision will be guided by the two goals of preserving the free status
-of all derivatives of our free software and of promoting the sharing
-and reuse of software generally.
-
-                            NO WARRANTY
-
-  15. BECAUSE THE LIBRARY IS LICENSED FREE OF CHARGE, THERE IS NO
-WARRANTY FOR THE LIBRARY, TO THE EXTENT PERMITTED BY APPLICABLE LAW.
-EXCEPT WHEN OTHERWISE STATED IN WRITING THE COPYRIGHT HOLDERS AND/OR
-OTHER PARTIES PROVIDE THE LIBRARY "AS IS" WITHOUT WARRANTY OF ANY
-KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING, BUT NOT LIMITED TO, THE
-IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
-PURPOSE.  THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE OF THE
-LIBRARY IS WITH YOU.  SHOULD THE LIBRARY PROVE DEFECTIVE, YOU ASSUME
-THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
-
-  16. IN NO EVENT UNLESS REQUIRED BY APPLICABLE LAW OR AGREED TO IN
-WRITING WILL ANY COPYRIGHT HOLDER, OR ANY OTHER PARTY WHO MAY MODIFY
-AND/OR REDISTRIBUTE THE LIBRARY AS PERMITTED ABOVE, BE LIABLE TO YOU
-FOR DAMAGES, INCLUDING ANY GENERAL, SPECIAL, INCIDENTAL OR
-CONSEQUENTIAL DAMAGES ARISING OUT OF THE USE OR INABILITY TO USE THE
-LIBRARY (INCLUDING BUT NOT LIMITED TO LOSS OF DATA OR DATA BEING
-RENDERED INACCURATE OR LOSSES SUSTAINED BY YOU OR THIRD PARTIES OR A
-FAILURE OF THE LIBRARY TO OPERATE WITH ANY OTHER SOFTWARE), EVEN IF
-SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH
-DAMAGES.
-
-                     END OF TERMS AND CONDITIONS
-\f
-           How to Apply These Terms to Your New Libraries
-
-  If you develop a new library, and you want it to be of the greatest
-possible use to the public, we recommend making it free software that
-everyone can redistribute and change.  You can do so by permitting
-redistribution under these terms (or, alternatively, under the terms
-of the ordinary General Public License).
-
-  To apply these terms, attach the following notices to the library.
-It is safest to attach them to the start of each source file to most
-effectively convey the exclusion of warranty; and each file should
-have at least the "copyright" line and a pointer to where the full
-notice is found.
-
-
-    <one line to give the library's name and a brief idea of what it does.>
-    Copyright (C) <year>  <name of author>
-
-    This library is free software; you can redistribute it and/or
-    modify it under the terms of the GNU Lesser General Public
-    License as published by the Free Software Foundation; either
-    version 2.1 of the License, or (at your option) any later version.
-
-    This library is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-    Lesser General Public License for more details.
-
-    You should have received a copy of the GNU Lesser General Public
-    License along with this library; If not, see <http://www.gnu.org/licenses/>.
-
-Also add information on how to contact you by electronic and paper mail.
-
-You should also get your employer (if you work as a programmer) or
-your school, if any, to sign a "copyright disclaimer" for the library,
-if necessary.  Here is a sample; alter the names:
-
-  Yoyodyne, Inc., hereby disclaims all copyright interest in the
-  library `Frob' (a library for tweaking knobs) written by James
-  Random Hacker.
-
-  <signature of Ty Coon>, 1 April 1990
-  Ty Coon, President of Vice
-
-That's all there is to it!
-
-
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 56723139a1..dc39b6cb31 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -44,7 +44,7 @@ xenstored: $(XENSTORED_OBJS-y)
 $(CLIENTS): xenstore
 	ln -f xenstore $@
 
-xenstore: xenstore_client.o xs_lib.o
+xenstore: xenstore_client.o
 	$(CC) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(APPEND_LDFLAGS)
 
 xenstore-control: xenstore_control.o
diff --git a/tools/xenstore/Makefile.common b/tools/xenstore/Makefile.common
index b18f95c103..f71c9bfd55 100644
--- a/tools/xenstore/Makefile.common
+++ b/tools/xenstore/Makefile.common
@@ -2,7 +2,7 @@
 
 XENSTORED_OBJS-y := xenstored_core.o xenstored_watch.o xenstored_domain.o
 XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o
-XENSTORED_OBJS-y += xs_lib.o talloc.o utils.o tdb.o hashtable.o
+XENSTORED_OBJS-y += talloc.o utils.o tdb.o hashtable.o
 
 XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o
 XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o
diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 8ff8abf12a..96f97ac77e 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -23,7 +23,7 @@
 
 #include <sys/ioctl.h>
 
-#include "xs_lib.h"
+#include <xen-tools/xenstore-common.h>
 
 #define PATH_SEP '/'
 #define MAX_PATH_LEN 256
@@ -361,7 +361,7 @@ static void do_ls(struct xs_handle *h, char *path, int cur_depth, int show_perms
                 for (i = 0; i < nperms; i++) {
                     if (i)
                         putchar(',');
-                    xs_perm_to_string(perms+i, buf, sizeof(buf));
+                    xenstore_perm_to_string(perms+i, buf, sizeof(buf));
                     fputs(buf, stdout);
                 }
                 putchar(')');
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 6ef800ff64..0d131e2ebc 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -31,10 +31,10 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <xenctrl.h>
+#include <xen-tools/xenstore-common.h>
 
 #include "utils.h"
 #include "talloc.h"
-#include "xs_lib.h"
 #include "xenstored_core.h"
 #include "xenstored_control.h"
 #include "xenstored_domain.h"
@@ -560,7 +560,8 @@ static FILE *lu_dump_open(const void *ctx)
 	char *filename;
 	int fd;
 
-	filename = talloc_asprintf(ctx, "%s/state_dump", xs_daemon_rundir());
+	filename = talloc_asprintf(ctx, "%s/state_dump",
+				   xenstore_daemon_rundir());
 	if (!filename)
 		return NULL;
 
@@ -583,7 +584,7 @@ static void lu_get_dump_state(struct lu_dump_state *state)
 	state->size = 0;
 
 	state->filename = talloc_asprintf(NULL, "%s/state_dump",
-					  xs_daemon_rundir());
+					  xenstore_daemon_rundir());
 	if (!state->filename)
 		barf("Allocation failure");
 
@@ -1017,7 +1018,7 @@ int do_control(const void *ctx, struct connection *conn,
 	if (cmd == ARRAY_SIZE(cmds))
 		return EINVAL;
 
-	num = xs_count_strings(in->buffer, in->used);
+	num = xenstore_count_strings(in->buffer, in->used);
 	if (cmds[cmd].max_pars)
 		num = min(num, cmds[cmd].max_pars);
 	vec = talloc_array(ctx, char *, num);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index a78cbf1116..62deee9cb9 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -42,11 +42,11 @@
 #include <setjmp.h>
 
 #include <xenevtchn.h>
+#include <xen-tools/xenstore-common.h>
 
 #include "utils.h"
 #include "list.h"
 #include "talloc.h"
-#include "xs_lib.h"
 #include "xenstored_core.h"
 #include "xenstored_watch.h"
 #include "xenstored_transaction.h"
@@ -1201,7 +1201,8 @@ static char *perms_to_strings(const void *ctx, const struct node_perms *perms,
 	char buffer[MAX_STRLEN(unsigned int) + 1];
 
 	for (*len = 0, i = 0; i < perms->num; i++) {
-		if (!xs_perm_to_string(&perms->p[i], buffer, sizeof(buffer)))
+		if (!xenstore_perm_to_string(&perms->p[i], buffer,
+					     sizeof(buffer)))
 			return NULL;
 
 		strings = talloc_realloc(ctx, strings, char,
@@ -1279,7 +1280,7 @@ static int send_directory_part(const void *ctx, struct connection *conn,
 	struct node *node;
 	char gen[24];
 
-	if (xs_count_strings(in->buffer, in->used) != 2)
+	if (xenstore_count_strings(in->buffer, in->used) != 2)
 		return EINVAL;
 
 	/* First arg is node name. */
@@ -1766,7 +1767,7 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 	char *name, *permstr;
 	struct node *node;
 
-	perms.num = xs_count_strings(in->buffer, in->used);
+	perms.num = xenstore_count_strings(in->buffer, in->used);
 	if (perms.num < 2)
 		return EINVAL;
 
@@ -1779,7 +1780,7 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 	perms.p = talloc_array(ctx, struct xs_permissions, perms.num);
 	if (!perms.p)
 		return ENOMEM;
-	if (!xs_strings_to_perms(perms.p, perms.num, permstr))
+	if (!xenstore_strings_to_perms(perms.p, perms.num, permstr))
 		return errno;
 
 	if (domain_alloc_permrefs(&perms) < 0)
@@ -2575,7 +2576,7 @@ static void destroy_fds(void)
 static void init_sockets(void)
 {
 	struct sockaddr_un addr;
-	const char *soc_str = xs_daemon_socket();
+	const char *soc_str = xenstore_daemon_path();
 
 	if (!soc_str)
 		barf_perror("Failed to obtain xs domain socket");
@@ -2890,7 +2891,7 @@ int main(int argc, char *argv[])
 
 	/* Make sure xenstored directory exists. */
 	/* Errors ignored here, will be reported when we open files */
-	mkdir(xs_daemon_rundir(), 0755);
+	mkdir(xenstore_daemon_rundir(), 0755);
 
 	if (dofork) {
 		openlog("xenstored", 0, LOG_DAEMON);
diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
deleted file mode 100644
index 826fa7b5a8..0000000000
--- a/tools/xenstore/xs_lib.c
+++ /dev/null
@@ -1,126 +0,0 @@
-/* 
-    Common routines between Xen store user library and daemon.
-    Copyright (C) 2005 Rusty Russell IBM Corporation
-
-    This library is free software; you can redistribute it and/or
-    modify it under the terms of the GNU Lesser General Public
-    License as published by the Free Software Foundation; either
-    version 2.1 of the License, or (at your option) any later version.
-
-    This library is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-    Lesser General Public License for more details.
-
-    You should have received a copy of the GNU Lesser General Public
-    License along with this library; If not, see <http://www.gnu.org/licenses/>.
-*/
-
-#include <assert.h>
-#include <unistd.h>
-#include <stdio.h>
-#include <string.h>
-#include <stdlib.h>
-#include <errno.h>
-#include "xs_lib.h"
-
-/* Common routines for the Xen store daemon and client library. */
-
-const char *xs_daemon_rundir(void)
-{
-	char *s = getenv("XENSTORED_RUNDIR");
-	return (s ? s : XEN_RUN_STORED);
-}
-
-static const char *xs_daemon_path(void)
-{
-	static char buf[PATH_MAX];
-	char *s = getenv("XENSTORED_PATH");
-	if (s)
-		return s;
-	if (snprintf(buf, sizeof(buf), "%s/socket",
-		     xs_daemon_rundir()) >= PATH_MAX)
-		return NULL;
-	return buf;
-}
-
-const char *xs_daemon_socket(void)
-{
-	return xs_daemon_path();
-}
-
-/* Convert strings to permissions.  False if a problem. */
-bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
-			 const char *strings)
-{
-	const char *p;
-	char *end;
-	unsigned int i;
-
-	for (p = strings, i = 0; i < num; i++) {
-		/* "r", "w", or "b" for both. */
-		switch (*p) {
-		case 'r':
-			perms[i].perms = XS_PERM_READ;
-			break;
-		case 'w':
-			perms[i].perms = XS_PERM_WRITE;
-			break;
-		case 'b':
-			perms[i].perms = XS_PERM_READ|XS_PERM_WRITE;
-			break;
-		case 'n':
-			perms[i].perms = XS_PERM_NONE;
-			break;
-		default:
-			errno = EINVAL;
-			return false;
-		} 
-		p++;
-		perms[i].id = strtol(p, &end, 0);
-		if (*end || !*p) {
-			errno = EINVAL;
-			return false;
-		}
-		p = end + 1;
-	}
-	return true;
-}
-
-/* Convert permissions to a string (up to len MAX_STRLEN(unsigned int)+1). */
-bool xs_perm_to_string(const struct xs_permissions *perm,
-                       char *buffer, size_t buf_len)
-{
-	switch ((int)perm->perms & ~XS_PERM_IGNORE) {
-	case XS_PERM_WRITE:
-		*buffer = 'w';
-		break;
-	case XS_PERM_READ:
-		*buffer = 'r';
-		break;
-	case XS_PERM_READ|XS_PERM_WRITE:
-		*buffer = 'b';
-		break;
-	case XS_PERM_NONE:
-		*buffer = 'n';
-		break;
-	default:
-		errno = EINVAL;
-		return false;
-	}
-	snprintf(buffer+1, buf_len-1, "%i", (int)perm->id);
-	return true;
-}
-
-/* Given a string and a length, count how many strings (nul terms). */
-unsigned int xs_count_strings(const char *strings, unsigned int len)
-{
-	unsigned int num;
-	const char *p;
-
-	for (p = strings, num = 0; p < strings + len; p++)
-		if (*p == '\0')
-			num++;
-
-	return num;
-}
diff --git a/tools/xenstore/xs_lib.h b/tools/xenstore/xs_lib.h
deleted file mode 100644
index 5b9874d741..0000000000
--- a/tools/xenstore/xs_lib.h
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
-    Common routines between Xen store user library and daemon.
-    Copyright (C) 2005 Rusty Russell IBM Corporation
-
-    This library is free software; you can redistribute it and/or
-    modify it under the terms of the GNU Lesser General Public
-    License as published by the Free Software Foundation; either
-    version 2.1 of the License, or (at your option) any later version.
-
-    This library is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-    Lesser General Public License for more details.
-
-    You should have received a copy of the GNU Lesser General Public
-    License along with this library; If not, see <http://www.gnu.org/licenses/>.
-*/
-
-#ifndef XS_LIB_H
-#define XS_LIB_H
-
-#include "xenstore_lib.h"
-
-/* Convert permissions to a string (up to len MAX_STRLEN(unsigned int)+1). */
-bool xs_perm_to_string(const struct xs_permissions *perm,
-		       char *buffer, size_t buf_len);
-
-/* Given a string and a length, count how many strings (nul terms). */
-unsigned int xs_count_strings(const char *strings, unsigned int len);
-
-#endif /* XS_LIB_H */
-- 
2.35.3

[PATCH v3 14/16] tools/xenstore: split out environment specific live update code

[Juergen Gross]

Instead of using #ifdef in xenstored_control.c split out the code of
environment specific functions (daemon or Mini-OS) to dedicated source
files.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/Makefile.common       |   8 +-
 tools/xenstore/xenstored_control.c   | 253 +--------------------------
 tools/xenstore/xenstored_lu.h        |  56 ++++++
 tools/xenstore/xenstored_lu_daemon.c | 133 ++++++++++++++
 tools/xenstore/xenstored_lu_minios.c | 121 +++++++++++++
 5 files changed, 317 insertions(+), 254 deletions(-)
 create mode 100644 tools/xenstore/xenstored_lu.h
 create mode 100644 tools/xenstore/xenstored_lu_daemon.c
 create mode 100644 tools/xenstore/xenstored_lu_minios.c

diff --git a/tools/xenstore/Makefile.common b/tools/xenstore/Makefile.common
index f71c9bfd55..c42796fe34 100644
--- a/tools/xenstore/Makefile.common
+++ b/tools/xenstore/Makefile.common
@@ -4,10 +4,10 @@ XENSTORED_OBJS-y := xenstored_core.o xenstored_watch.o xenstored_domain.o
 XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o
 XENSTORED_OBJS-y += talloc.o utils.o tdb.o hashtable.o
 
-XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_FreeBSD) += xenstored_posix.o
-XENSTORED_OBJS-$(CONFIG_MiniOS) += xenstored_minios.o
+XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_FreeBSD) += xenstored_posix.o xenstored_lu_daemon.o
+XENSTORED_OBJS-$(CONFIG_MiniOS) += xenstored_minios.o xenstored_lu_minios.o
 
 # Include configure output (config.h)
 CFLAGS += -include $(XEN_ROOT)/tools/config.h
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 0d131e2ebc..b61b41f16c 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -38,63 +38,13 @@
 #include "xenstored_core.h"
 #include "xenstored_control.h"
 #include "xenstored_domain.h"
+#include "xenstored_lu.h"
 #include "xenstored_watch.h"
 
-/* Mini-OS only knows about MAP_ANON. */
-#ifndef MAP_ANONYMOUS
-#define MAP_ANONYMOUS MAP_ANON
-#endif
-
 #ifndef NO_LIVE_UPDATE
-struct live_update {
-	/* For verification the correct connection is acting. */
-	struct connection *conn;
-
-	/* Pointer to the command used to request LU */
-	struct buffered_data *in;
+struct live_update *lu_status;
 
-#ifdef __MINIOS__
-	void *kernel;
-	unsigned int kernel_size;
-	unsigned int kernel_off;
-
-	void *dump_state;
-	unsigned long dump_size;
-#else
-	char *filename;
-#endif
-
-	char *cmdline;
-
-	/* Start parameters. */
-	bool force;
-	unsigned int timeout;
-	time_t started_at;
-};
-
-static struct live_update *lu_status;
-
-struct lu_dump_state {
-	void *buf;
-	unsigned int size;
-#ifndef __MINIOS__
-	int fd;
-	char *filename;
-#endif
-};
-
-static int lu_destroy(void *data)
-{
-#ifdef __MINIOS__
-	if (lu_status->dump_state)
-		munmap(lu_status->dump_state, lu_status->dump_size);
-#endif
-	lu_status = NULL;
-
-	return 0;
-}
-
-static const char *lu_begin(struct connection *conn)
+const char *lu_begin(struct connection *conn)
 {
 	if (lu_status)
 		return "live-update session already active.";
@@ -431,203 +381,6 @@ static const char *lu_cmdline(const void *ctx, struct connection *conn,
 	return NULL;
 }
 
-#ifdef __MINIOS__
-static const char *lu_binary_alloc(const void *ctx, struct connection *conn,
-				   unsigned long size)
-{
-	const char *ret;
-
-	syslog(LOG_INFO, "live-update: binary size %lu\n", size);
-
-	ret = lu_begin(conn);
-	if (ret)
-		return ret;
-
-	lu_status->kernel = talloc_size(lu_status, size);
-	if (!lu_status->kernel)
-		return "Allocation failure.";
-
-	lu_status->kernel_size = size;
-	lu_status->kernel_off = 0;
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_binary_save(const void *ctx, struct connection *conn,
-				  unsigned int size, const char *data)
-{
-	if (!lu_status || lu_status->conn != conn)
-		return "Not in live-update session.";
-
-	if (lu_status->kernel_off + size > lu_status->kernel_size)
-		return "Too much kernel data.";
-
-	memcpy(lu_status->kernel + lu_status->kernel_off, data, size);
-	lu_status->kernel_off += size;
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_arch(const void *ctx, struct connection *conn,
-			   char **vec, int num)
-{
-	if (num == 2 && !strcmp(vec[0], "-b"))
-		return lu_binary_alloc(ctx, conn, atol(vec[1]));
-	if (num > 2 && !strcmp(vec[0], "-d"))
-		return lu_binary_save(ctx, conn, atoi(vec[1]), vec[2]);
-
-	errno = EINVAL;
-	return NULL;
-}
-
-static FILE *lu_dump_open(const void *ctx)
-{
-	lu_status->dump_size = ROUNDUP(talloc_total_size(NULL) * 2,
-				       XC_PAGE_SHIFT);
-	lu_status->dump_state = mmap(NULL, lu_status->dump_size,
-				     PROT_READ | PROT_WRITE,
-				     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-	if (lu_status->dump_state == MAP_FAILED)
-		return NULL;
-
-	return fmemopen(lu_status->dump_state, lu_status->dump_size, "w");
-}
-
-static void lu_dump_close(FILE *fp)
-{
-	size_t size;
-
-	size = ftell(fp);
-	size = ROUNDUP(size, XC_PAGE_SHIFT);
-	munmap(lu_status->dump_state + size, lu_status->dump_size - size);
-	lu_status->dump_size = size;
-
-	fclose(fp);
-}
-
-static void lu_get_dump_state(struct lu_dump_state *state)
-{
-}
-
-static void lu_close_dump_state(struct lu_dump_state *state)
-{
-}
-
-static char *lu_exec(const void *ctx, int argc, char **argv)
-{
-	return "NYI";
-}
-#else
-static const char *lu_binary(const void *ctx, struct connection *conn,
-			     const char *filename)
-{
-	const char *ret;
-	struct stat statbuf;
-
-	syslog(LOG_INFO, "live-update: binary %s\n", filename);
-
-	if (stat(filename, &statbuf))
-		return "File not accessible.";
-	if (!(statbuf.st_mode & (S_IXOTH | S_IXGRP | S_IXUSR)))
-		return "File not executable.";
-
-	ret = lu_begin(conn);
-	if (ret)
-		return ret;
-
-	lu_status->filename = talloc_strdup(lu_status, filename);
-	if (!lu_status->filename)
-		return "Allocation failure.";
-
-	errno = 0;
-	return NULL;
-}
-
-static const char *lu_arch(const void *ctx, struct connection *conn,
-			   char **vec, int num)
-{
-	if (num == 2 && !strcmp(vec[0], "-f"))
-		return lu_binary(ctx, conn, vec[1]);
-
-	errno = EINVAL;
-	return NULL;
-}
-
-static FILE *lu_dump_open(const void *ctx)
-{
-	char *filename;
-	int fd;
-
-	filename = talloc_asprintf(ctx, "%s/state_dump",
-				   xenstore_daemon_rundir());
-	if (!filename)
-		return NULL;
-
-	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
-	if (fd < 0)
-		return NULL;
-
-	return fdopen(fd, "w");
-}
-
-static void lu_dump_close(FILE *fp)
-{
-	fclose(fp);
-}
-
-static void lu_get_dump_state(struct lu_dump_state *state)
-{
-	struct stat statbuf;
-
-	state->size = 0;
-
-	state->filename = talloc_asprintf(NULL, "%s/state_dump",
-					  xenstore_daemon_rundir());
-	if (!state->filename)
-		barf("Allocation failure");
-
-	state->fd = open(state->filename, O_RDONLY);
-	if (state->fd < 0)
-		return;
-	if (fstat(state->fd, &statbuf) != 0)
-		goto out_close;
-	state->size = statbuf.st_size;
-
-	state->buf = mmap(NULL, state->size, PROT_READ, MAP_PRIVATE,
-			  state->fd, 0);
-	if (state->buf == MAP_FAILED) {
-		state->size = 0;
-		goto out_close;
-	}
-
-	return;
-
- out_close:
-	close(state->fd);
-}
-
-static void lu_close_dump_state(struct lu_dump_state *state)
-{
-	assert(state->filename != NULL);
-
-	munmap(state->buf, state->size);
-	close(state->fd);
-
-	unlink(state->filename);
-	talloc_free(state->filename);
-}
-
-static char *lu_exec(const void *ctx, int argc, char **argv)
-{
-	argv[0] = lu_status->filename;
-	execvp(argv[0], argv);
-
-	return "Error activating new binary.";
-}
-#endif
-
 static bool lu_check_lu_allowed(void)
 {
 	struct connection *conn;
diff --git a/tools/xenstore/xenstored_lu.h b/tools/xenstore/xenstored_lu.h
new file mode 100644
index 0000000000..d2f8e4e57c
--- /dev/null
+++ b/tools/xenstore/xenstored_lu.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Live Update interfaces for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#ifndef NO_LIVE_UPDATE
+struct live_update {
+	/* For verification the correct connection is acting. */
+	struct connection *conn;
+
+	/* Pointer to the command used to request LU */
+	struct buffered_data *in;
+
+#ifdef __MINIOS__
+	void *kernel;
+	unsigned int kernel_size;
+	unsigned int kernel_off;
+
+	void *dump_state;
+	unsigned long dump_size;
+#else
+	char *filename;
+#endif
+
+	char *cmdline;
+
+	/* Start parameters. */
+	bool force;
+	unsigned int timeout;
+	time_t started_at;
+};
+
+struct lu_dump_state {
+	void *buf;
+	unsigned int size;
+#ifndef __MINIOS__
+	int fd;
+	char *filename;
+#endif
+};
+
+extern struct live_update *lu_status;
+
+/* Live update private interfaces. */
+void lu_get_dump_state(struct lu_dump_state *state);
+void lu_close_dump_state(struct lu_dump_state *state);
+FILE *lu_dump_open(const void *ctx);
+void lu_dump_close(FILE *fp);
+char *lu_exec(const void *ctx, int argc, char **argv);
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num);
+const char *lu_begin(struct connection *conn);
+int lu_destroy(void *data);
+#endif
diff --git a/tools/xenstore/xenstored_lu_daemon.c b/tools/xenstore/xenstored_lu_daemon.c
new file mode 100644
index 0000000000..952d9f0b25
--- /dev/null
+++ b/tools/xenstore/xenstored_lu_daemon.c
@@ -0,0 +1,133 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * Live Update for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <syslog.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <xen-tools/xenstore-common.h>
+
+#include "talloc.h"
+#include "xenstored_core.h"
+#include "xenstored_lu.h"
+
+#ifndef NO_LIVE_UPDATE
+void lu_get_dump_state(struct lu_dump_state *state)
+{
+	struct stat statbuf;
+
+	state->size = 0;
+
+	state->filename = talloc_asprintf(NULL, "%s/state_dump",
+					  xenstore_daemon_rundir());
+	if (!state->filename)
+		barf("Allocation failure");
+
+	state->fd = open(state->filename, O_RDONLY);
+	if (state->fd < 0)
+		return;
+	if (fstat(state->fd, &statbuf) != 0)
+		goto out_close;
+	state->size = statbuf.st_size;
+
+	state->buf = mmap(NULL, state->size, PROT_READ, MAP_PRIVATE,
+			  state->fd, 0);
+	if (state->buf == MAP_FAILED) {
+		state->size = 0;
+		goto out_close;
+	}
+
+	return;
+
+ out_close:
+	close(state->fd);
+}
+
+void lu_close_dump_state(struct lu_dump_state *state)
+{
+	assert(state->filename != NULL);
+
+	munmap(state->buf, state->size);
+	close(state->fd);
+
+	unlink(state->filename);
+	talloc_free(state->filename);
+}
+
+FILE *lu_dump_open(const void *ctx)
+{
+	char *filename;
+	int fd;
+
+	filename = talloc_asprintf(ctx, "%s/state_dump",
+				   xenstore_daemon_rundir());
+	if (!filename)
+		return NULL;
+
+	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
+	if (fd < 0)
+		return NULL;
+
+	return fdopen(fd, "w");
+}
+
+void lu_dump_close(FILE *fp)
+{
+	fclose(fp);
+}
+
+char *lu_exec(const void *ctx, int argc, char **argv)
+{
+	argv[0] = lu_status->filename;
+	execvp(argv[0], argv);
+
+	return "Error activating new binary.";
+}
+
+int lu_destroy(void *data)
+{
+	lu_status = NULL;
+
+	return 0;
+}
+
+static const char *lu_binary(const void *ctx, struct connection *conn,
+			     const char *filename)
+{
+	const char *ret;
+	struct stat statbuf;
+
+	syslog(LOG_INFO, "live-update: binary %s\n", filename);
+
+	if (stat(filename, &statbuf))
+		return "File not accessible.";
+	if (!(statbuf.st_mode & (S_IXOTH | S_IXGRP | S_IXUSR)))
+		return "File not executable.";
+
+	ret = lu_begin(conn);
+	if (ret)
+		return ret;
+
+	lu_status->filename = talloc_strdup(lu_status, filename);
+	if (!lu_status->filename)
+		return "Allocation failure.";
+
+	errno = 0;
+	return NULL;
+}
+
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num)
+{
+	if (num == 2 && !strcmp(vec[0], "-f"))
+		return lu_binary(ctx, conn, vec[1]);
+
+	errno = EINVAL;
+	return NULL;
+}
+#endif
diff --git a/tools/xenstore/xenstored_lu_minios.c b/tools/xenstore/xenstored_lu_minios.c
new file mode 100644
index 0000000000..0bec8a0037
--- /dev/null
+++ b/tools/xenstore/xenstored_lu_minios.c
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * Live Update for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <syslog.h>
+#include <sys/mman.h>
+#include <xenctrl.h>
+#include <xen-tools/common-macros.h>
+
+#include "talloc.h"
+#include "xenstored_lu.h"
+
+/* Mini-OS only knows about MAP_ANON. */
+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+
+#ifndef NO_LIVE_UPDATE
+void lu_get_dump_state(struct lu_dump_state *state)
+{
+}
+
+void lu_close_dump_state(struct lu_dump_state *state)
+{
+}
+
+FILE *lu_dump_open(const void *ctx)
+{
+	lu_status->dump_size = ROUNDUP(talloc_total_size(NULL) * 2,
+				       XC_PAGE_SHIFT);
+	lu_status->dump_state = mmap(NULL, lu_status->dump_size,
+				     PROT_READ | PROT_WRITE,
+				     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (lu_status->dump_state == MAP_FAILED)
+		return NULL;
+
+	return fmemopen(lu_status->dump_state, lu_status->dump_size, "w");
+}
+
+void lu_dump_close(FILE *fp)
+{
+	size_t size;
+
+	size = ftell(fp);
+	size = ROUNDUP(size, XC_PAGE_SHIFT);
+	munmap(lu_status->dump_state + size, lu_status->dump_size - size);
+	lu_status->dump_size = size;
+
+	fclose(fp);
+}
+
+char *lu_exec(const void *ctx, int argc, char **argv)
+{
+	return "NYI";
+}
+
+int lu_destroy(void *data)
+{
+	if (lu_status->dump_state)
+		munmap(lu_status->dump_state, lu_status->dump_size);
+	lu_status = NULL;
+
+	return 0;
+}
+
+static const char *lu_binary_alloc(const void *ctx, struct connection *conn,
+				   unsigned long size)
+{
+	const char *ret;
+
+	syslog(LOG_INFO, "live-update: binary size %lu\n", size);
+
+	ret = lu_begin(conn);
+	if (ret)
+		return ret;
+
+	lu_status->kernel = talloc_size(lu_status, size);
+	if (!lu_status->kernel)
+		return "Allocation failure.";
+
+	lu_status->kernel_size = size;
+	lu_status->kernel_off = 0;
+
+	errno = 0;
+	return NULL;
+}
+
+static const char *lu_binary_save(const void *ctx, struct connection *conn,
+				  unsigned int size, const char *data)
+{
+	if (!lu_status || lu_status->conn != conn)
+		return "Not in live-update session.";
+
+	if (lu_status->kernel_off + size > lu_status->kernel_size)
+		return "Too much kernel data.";
+
+	memcpy(lu_status->kernel + lu_status->kernel_off, data, size);
+	lu_status->kernel_off += size;
+
+	errno = 0;
+	return NULL;
+}
+
+const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
+		    int num)
+{
+	if (num == 2 && !strcmp(vec[0], "-b"))
+		return lu_binary_alloc(ctx, conn, atol(vec[1]));
+	if (num > 2 && !strcmp(vec[0], "-d"))
+		return lu_binary_save(ctx, conn, atoi(vec[1]), vec[2]);
+
+	errno = EINVAL;
+	return NULL;
+}
+#endif
-- 
2.35.3

[PATCH v3 15/16] tools/xenstore: split out rest of live update control code

[Juergen Gross]

Move the rest of live update related code from xenstored_control.c to
a dedicated new source file.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/Makefile.common     |   2 +-
 tools/xenstore/xenstored_control.c | 409 -----------------------------
 tools/xenstore/xenstored_control.h |   8 -
 tools/xenstore/xenstored_core.c    |   1 +
 tools/xenstore/xenstored_lu.c      | 400 ++++++++++++++++++++++++++++
 tools/xenstore/xenstored_lu.h      |  25 ++
 6 files changed, 427 insertions(+), 418 deletions(-)
 create mode 100644 tools/xenstore/xenstored_lu.c

diff --git a/tools/xenstore/Makefile.common b/tools/xenstore/Makefile.common
index c42796fe34..657a16849e 100644
--- a/tools/xenstore/Makefile.common
+++ b/tools/xenstore/Makefile.common
@@ -1,7 +1,7 @@
 # Makefile shared with stubdom
 
 XENSTORED_OBJS-y := xenstored_core.o xenstored_watch.o xenstored_domain.o
-XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o
+XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o xenstored_lu.o
 XENSTORED_OBJS-y += talloc.o utils.o tdb.o hashtable.o
 
 XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o xenstored_lu_daemon.o
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index b61b41f16c..145a0e5aff 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -16,21 +16,13 @@
     along with this program; If not, see <http://www.gnu.org/licenses/>.
 */
 
-#include <assert.h>
-#include <ctype.h>
 #include <errno.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <syslog.h>
-#include <time.h>
 #include <sys/types.h>
-#include <sys/stat.h>
-#include <sys/mman.h>
-#include <fcntl.h>
 #include <unistd.h>
-#include <xenctrl.h>
 #include <xen-tools/xenstore-common.h>
 
 #include "utils.h"
@@ -39,69 +31,6 @@
 #include "xenstored_control.h"
 #include "xenstored_domain.h"
 #include "xenstored_lu.h"
-#include "xenstored_watch.h"
-
-#ifndef NO_LIVE_UPDATE
-struct live_update *lu_status;
-
-const char *lu_begin(struct connection *conn)
-{
-	if (lu_status)
-		return "live-update session already active.";
-
-	lu_status = talloc_zero(conn, struct live_update);
-	if (!lu_status)
-		return "Allocation failure.";
-	lu_status->conn = conn;
-	talloc_set_destructor(lu_status, lu_destroy);
-
-	return NULL;
-}
-
-struct connection *lu_get_connection(void)
-{
-	return lu_status ? lu_status->conn : NULL;
-}
-
-unsigned int lu_write_response(FILE *fp)
-{
-	struct xsd_sockmsg msg;
-
-	assert(lu_status);
-
-	msg = lu_status->in->hdr.msg;
-
-	msg.len = sizeof("OK");
-	if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
-		return 0;
-	if (fp && fwrite("OK", msg.len, 1, fp) != 1)
-		return 0;
-
-	return sizeof(msg) + msg.len;
-}
-
-bool lu_is_pending(void)
-{
-	return lu_status != NULL;
-}
-
-#else
-struct connection *lu_get_connection(void)
-{
-	return NULL;
-}
-
-unsigned int lu_write_response(FILE *fp)
-{
-	/* Unsupported */
-	return 0;
-}
-
-bool lu_is_pending(void)
-{
-	return false;
-}
-#endif
 
 struct cmd_s {
 	char *cmd;
@@ -352,344 +281,6 @@ static int do_control_print(const void *ctx, struct connection *conn,
 	return 0;
 }
 
-#ifndef NO_LIVE_UPDATE
-static const char *lu_abort(const void *ctx, struct connection *conn)
-{
-	syslog(LOG_INFO, "live-update: abort\n");
-
-	if (!lu_status)
-		return "No live-update session active.";
-
-	/* Destructor will do the real abort handling. */
-	talloc_free(lu_status);
-
-	return NULL;
-}
-
-static const char *lu_cmdline(const void *ctx, struct connection *conn,
-			      const char *cmdline)
-{
-	syslog(LOG_INFO, "live-update: cmdline %s\n", cmdline);
-
-	if (!lu_status || lu_status->conn != conn)
-		return "Not in live-update session.";
-
-	lu_status->cmdline = talloc_strdup(lu_status, cmdline);
-	if (!lu_status->cmdline)
-		return "Allocation failure.";
-
-	return NULL;
-}
-
-static bool lu_check_lu_allowed(void)
-{
-	struct connection *conn;
-	time_t now = time(NULL);
-	unsigned int ta_total = 0, ta_long = 0;
-
-	list_for_each_entry(conn, &connections, list) {
-		if (conn->ta_start_time) {
-			ta_total++;
-			if (now - conn->ta_start_time >= lu_status->timeout)
-				ta_long++;
-		}
-	}
-
-	/*
-	 * Allow LiveUpdate if one of the following conditions is met:
-	 *	- There is no active transactions
-	 *	- All transactions are long running (e.g. they have been
-	 *	active for more than lu_status->timeout sec) and the admin as
-	 *	requested to force the operation.
-	 */
-	return ta_total ? (lu_status->force && ta_long == ta_total) : true;
-}
-
-static const char *lu_reject_reason(const void *ctx)
-{
-	char *ret = NULL;
-	struct connection *conn;
-	time_t now = time(NULL);
-
-	list_for_each_entry(conn, &connections, list) {
-		unsigned long tdiff = now - conn->ta_start_time;
-
-		if (conn->ta_start_time && (tdiff >= lu_status->timeout)) {
-			ret = talloc_asprintf(ctx, "%s\nDomain %u: %ld s",
-					      ret ? : "Domains with long running transactions:",
-					      conn->id, tdiff);
-		}
-	}
-
-	return ret ? (const char *)ret : "Overlapping transactions";
-}
-
-static const char *lu_dump_state(const void *ctx, struct connection *conn)
-{
-	FILE *fp;
-	const char *ret;
-	struct xs_state_record_header end;
-	struct xs_state_preamble pre;
-
-	fp = lu_dump_open(ctx);
-	if (!fp)
-		return "Dump state open error";
-
-	memcpy(pre.ident, XS_STATE_IDENT, sizeof(pre.ident));
-	pre.version = htobe32(XS_STATE_VERSION);
-	pre.flags = XS_STATE_FLAGS;
-	if (fwrite(&pre, sizeof(pre), 1, fp) != 1) {
-		ret = "Dump write error";
-		goto out;
-	}
-
-	ret = dump_state_global(fp);
-	if (ret)
-		goto out;
-	ret = dump_state_connections(fp);
-	if (ret)
-		goto out;
-	ret = dump_state_nodes(fp, ctx);
-	if (ret)
-		goto out;
-
-	end.type = XS_STATE_TYPE_END;
-	end.length = 0;
-	if (fwrite(&end, sizeof(end), 1, fp) != 1)
-		ret = "Dump write error";
-
- out:
-	lu_dump_close(fp);
-
-	return ret;
-}
-
-void lu_read_state(void)
-{
-	struct lu_dump_state state = {};
-	struct xs_state_record_header *head;
-	void *ctx = talloc_new(NULL); /* Work context for subfunctions. */
-	struct xs_state_preamble *pre;
-
-	syslog(LOG_INFO, "live-update: read state\n");
-	lu_get_dump_state(&state);
-	if (state.size == 0)
-		barf_perror("No state found after live-update");
-
-	pre = state.buf;
-	if (memcmp(pre->ident, XS_STATE_IDENT, sizeof(pre->ident)) ||
-	    pre->version != htobe32(XS_STATE_VERSION) ||
-	    pre->flags != XS_STATE_FLAGS)
-		barf("Unknown record identifier");
-	for (head = state.buf + sizeof(*pre);
-	     head->type != XS_STATE_TYPE_END &&
-		(void *)head - state.buf < state.size;
-	     head = (void *)head + sizeof(*head) + head->length) {
-		switch (head->type) {
-		case XS_STATE_TYPE_GLOBAL:
-			read_state_global(ctx, head + 1);
-			break;
-		case XS_STATE_TYPE_CONN:
-			read_state_connection(ctx, head + 1);
-			break;
-		case XS_STATE_TYPE_WATCH:
-			read_state_watch(ctx, head + 1);
-			break;
-		case XS_STATE_TYPE_TA:
-			xprintf("live-update: ignore transaction record\n");
-			break;
-		case XS_STATE_TYPE_NODE:
-			read_state_node(ctx, head + 1);
-			break;
-		default:
-			xprintf("live-update: unknown state record %08x\n",
-				head->type);
-			break;
-		}
-	}
-
-	lu_close_dump_state(&state);
-
-	talloc_free(ctx);
-
-	/*
-	 * We may have missed the VIRQ_DOM_EXC notification and a domain may
-	 * have died while we were live-updating. So check all the domains are
-	 * still alive.
-	 */
-	check_domains();
-}
-
-static const char *lu_activate_binary(const void *ctx)
-{
-	int argc;
-	char **argv;
-	unsigned int i;
-
-	if (lu_status->cmdline) {
-		argc = 4;   /* At least one arg + progname + "-U" + NULL. */
-		for (i = 0; lu_status->cmdline[i]; i++)
-			if (isspace(lu_status->cmdline[i]))
-				argc++;
-		argv = talloc_array(ctx, char *, argc);
-		if (!argv)
-			return "Allocation failure.";
-
-		i = 0;
-		argc = 1;
-		argv[1] = strtok(lu_status->cmdline, " \t");
-		while (argv[argc]) {
-			if (!strcmp(argv[argc], "-U"))
-				i = 1;
-			argc++;
-			argv[argc] = strtok(NULL, " \t");
-		}
-
-		if (!i) {
-			argv[argc++] = "-U";
-			argv[argc] = NULL;
-		}
-	} else {
-		for (i = 0; i < orig_argc; i++)
-			if (!strcmp(orig_argv[i], "-U"))
-				break;
-
-		argc = orig_argc;
-		argv = talloc_array(ctx, char *, orig_argc + 2);
-		if (!argv)
-			return "Allocation failure.";
-
-		memcpy(argv, orig_argv, orig_argc * sizeof(*argv));
-		if (i == orig_argc)
-			argv[argc++] = "-U";
-		argv[argc] = NULL;
-	}
-
-	domain_deinit();
-
-	return lu_exec(ctx, argc, argv);
-}
-
-static bool do_lu_start(struct delayed_request *req)
-{
-	time_t now = time(NULL);
-	const char *ret;
-	struct buffered_data *saved_in;
-	struct connection *conn = req->data;
-
-	/*
-	 * Cancellation may have been requested asynchronously. In this
-	 * case, lu_status will be NULL.
-	 */
-	if (!lu_status) {
-		ret = "Cancellation was requested";
-		goto out;
-	}
-
-	assert(lu_status->conn == conn);
-
-	if (!lu_check_lu_allowed()) {
-		if (now < lu_status->started_at + lu_status->timeout)
-			return false;
-		if (!lu_status->force) {
-			ret = lu_reject_reason(req);
-			goto out;
-		}
-	}
-
-	assert(req->in == lu_status->in);
-	/* Dump out internal state, including "OK" for live update. */
-	ret = lu_dump_state(req->in, conn);
-	if (!ret) {
-		/* Perform the activation of new binary. */
-		ret = lu_activate_binary(req->in);
-	}
-
-	/* We will reach this point only in case of failure. */
- out:
-	/*
-	 * send_reply() will send the response for conn->in. Save the current
-	 * conn->in and restore it afterwards.
-	 */
-	saved_in = conn->in;
-	conn->in = req->in;
-	send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
-	conn->in = saved_in;
-	talloc_free(lu_status);
-
-	return true;
-}
-
-static const char *lu_start(const void *ctx, struct connection *conn,
-			    bool force, unsigned int to)
-{
-	syslog(LOG_INFO, "live-update: start, force=%d, to=%u\n", force, to);
-
-	if (!lu_status || lu_status->conn != conn)
-		return "Not in live-update session.";
-
-#ifdef __MINIOS__
-	if (lu_status->kernel_size != lu_status->kernel_off)
-		return "Kernel not complete.";
-#endif
-
-	lu_status->force = force;
-	lu_status->timeout = to;
-	lu_status->started_at = time(NULL);
-	lu_status->in = conn->in;
-
-	errno = delay_request(conn, conn->in, do_lu_start, conn, false);
-
-	return NULL;
-}
-
-static int do_control_lu(const void *ctx, struct connection *conn,
-			 char **vec, int num)
-{
-	const char *ret = NULL;
-	unsigned int i;
-	bool force = false;
-	unsigned int to = 0;
-
-	if (num < 1)
-		return EINVAL;
-
-	if (!strcmp(vec[0], "-a")) {
-		if (num == 1)
-			ret = lu_abort(ctx, conn);
-		else
-			return EINVAL;
-	} else if (!strcmp(vec[0], "-c")) {
-		if (num == 2)
-			ret = lu_cmdline(ctx, conn, vec[1]);
-		else
-			return EINVAL;
-	} else if (!strcmp(vec[0], "-s")) {
-		for (i = 1; i < num; i++) {
-			if (!strcmp(vec[i], "-F"))
-				force = true;
-			else if (!strcmp(vec[i], "-t") && i < num - 1) {
-				i++;
-				to = atoi(vec[i]);
-			} else
-				return EINVAL;
-		}
-		ret = lu_start(ctx, conn, force, to);
-		if (!ret)
-			return errno;
-	} else {
-		ret = lu_arch(ctx, conn, vec, num);
-		if (!ret && errno)
-			return errno;
-	}
-
-	if (!ret)
-		ret = "OK";
-	send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
-	return 0;
-}
-#endif
-
 static int do_control_help(const void *, struct connection *, char **, int);
 
 static struct cmd_s cmds[] = {
diff --git a/tools/xenstore/xenstored_control.h b/tools/xenstore/xenstored_control.h
index a8cb76559b..faa955968d 100644
--- a/tools/xenstore/xenstored_control.h
+++ b/tools/xenstore/xenstored_control.h
@@ -18,11 +18,3 @@
 
 int do_control(const void *ctx, struct connection *conn,
 	       struct buffered_data *in);
-void lu_read_state(void);
-
-struct connection *lu_get_connection(void);
-
-/* Write the "OK" response for the live-update command */
-unsigned int lu_write_response(FILE *fp);
-
-bool lu_is_pending(void);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 62deee9cb9..31a862b715 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -52,6 +52,7 @@
 #include "xenstored_transaction.h"
 #include "xenstored_domain.h"
 #include "xenstored_control.h"
+#include "xenstored_lu.h"
 #include "tdb.h"
 
 #ifndef NO_SOCKETS
diff --git a/tools/xenstore/xenstored_lu.c b/tools/xenstore/xenstored_lu.c
new file mode 100644
index 0000000000..783bfd6456
--- /dev/null
+++ b/tools/xenstore/xenstored_lu.c
@@ -0,0 +1,400 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * Live Update interfaces for Xen Store Daemon.
+ * Copyright (C) 2022 Juergen Gross, SUSE LLC
+ */
+
+#include <assert.h>
+#include <ctype.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <syslog.h>
+#include <time.h>
+
+#include "talloc.h"
+#include "xenstored_core.h"
+#include "xenstored_domain.h"
+#include "xenstored_lu.h"
+#include "xenstored_watch.h"
+
+#ifndef NO_LIVE_UPDATE
+struct live_update *lu_status;
+
+const char *lu_begin(struct connection *conn)
+{
+	if (lu_status)
+		return "live-update session already active.";
+
+	lu_status = talloc_zero(conn, struct live_update);
+	if (!lu_status)
+		return "Allocation failure.";
+	lu_status->conn = conn;
+	talloc_set_destructor(lu_status, lu_destroy);
+
+	return NULL;
+}
+
+struct connection *lu_get_connection(void)
+{
+	return lu_status ? lu_status->conn : NULL;
+}
+
+unsigned int lu_write_response(FILE *fp)
+{
+	struct xsd_sockmsg msg;
+
+	assert(lu_status);
+
+	msg = lu_status->in->hdr.msg;
+
+	msg.len = sizeof("OK");
+	if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
+		return 0;
+	if (fp && fwrite("OK", msg.len, 1, fp) != 1)
+		return 0;
+
+	return sizeof(msg) + msg.len;
+}
+
+bool lu_is_pending(void)
+{
+	return lu_status != NULL;
+}
+
+void lu_read_state(void)
+{
+	struct lu_dump_state state = {};
+	struct xs_state_record_header *head;
+	void *ctx = talloc_new(NULL); /* Work context for subfunctions. */
+	struct xs_state_preamble *pre;
+
+	syslog(LOG_INFO, "live-update: read state\n");
+	lu_get_dump_state(&state);
+	if (state.size == 0)
+		barf_perror("No state found after live-update");
+
+	pre = state.buf;
+	if (memcmp(pre->ident, XS_STATE_IDENT, sizeof(pre->ident)) ||
+	    pre->version != htobe32(XS_STATE_VERSION) ||
+	    pre->flags != XS_STATE_FLAGS)
+		barf("Unknown record identifier");
+	for (head = state.buf + sizeof(*pre);
+	     head->type != XS_STATE_TYPE_END &&
+		(void *)head - state.buf < state.size;
+	     head = (void *)head + sizeof(*head) + head->length) {
+		switch (head->type) {
+		case XS_STATE_TYPE_GLOBAL:
+			read_state_global(ctx, head + 1);
+			break;
+		case XS_STATE_TYPE_CONN:
+			read_state_connection(ctx, head + 1);
+			break;
+		case XS_STATE_TYPE_WATCH:
+			read_state_watch(ctx, head + 1);
+			break;
+		case XS_STATE_TYPE_TA:
+			xprintf("live-update: ignore transaction record\n");
+			break;
+		case XS_STATE_TYPE_NODE:
+			read_state_node(ctx, head + 1);
+			break;
+		default:
+			xprintf("live-update: unknown state record %08x\n",
+				head->type);
+			break;
+		}
+	}
+
+	lu_close_dump_state(&state);
+
+	talloc_free(ctx);
+
+	/*
+	 * We may have missed the VIRQ_DOM_EXC notification and a domain may
+	 * have died while we were live-updating. So check all the domains are
+	 * still alive.
+	 */
+	check_domains();
+}
+
+static const char *lu_abort(const void *ctx, struct connection *conn)
+{
+	syslog(LOG_INFO, "live-update: abort\n");
+
+	if (!lu_status)
+		return "No live-update session active.";
+
+	/* Destructor will do the real abort handling. */
+	talloc_free(lu_status);
+
+	return NULL;
+}
+
+static const char *lu_cmdline(const void *ctx, struct connection *conn,
+			      const char *cmdline)
+{
+	syslog(LOG_INFO, "live-update: cmdline %s\n", cmdline);
+
+	if (!lu_status || lu_status->conn != conn)
+		return "Not in live-update session.";
+
+	lu_status->cmdline = talloc_strdup(lu_status, cmdline);
+	if (!lu_status->cmdline)
+		return "Allocation failure.";
+
+	return NULL;
+}
+
+static bool lu_check_lu_allowed(void)
+{
+	struct connection *conn;
+	time_t now = time(NULL);
+	unsigned int ta_total = 0, ta_long = 0;
+
+	list_for_each_entry(conn, &connections, list) {
+		if (conn->ta_start_time) {
+			ta_total++;
+			if (now - conn->ta_start_time >= lu_status->timeout)
+				ta_long++;
+		}
+	}
+
+	/*
+	 * Allow LiveUpdate if one of the following conditions is met:
+	 *	- There is no active transactions
+	 *	- All transactions are long running (e.g. they have been
+	 *	active for more than lu_status->timeout sec) and the admin as
+	 *	requested to force the operation.
+	 */
+	return ta_total ? (lu_status->force && ta_long == ta_total) : true;
+}
+
+static const char *lu_reject_reason(const void *ctx)
+{
+	char *ret = NULL;
+	struct connection *conn;
+	time_t now = time(NULL);
+
+	list_for_each_entry(conn, &connections, list) {
+		unsigned long tdiff = now - conn->ta_start_time;
+
+		if (conn->ta_start_time && (tdiff >= lu_status->timeout)) {
+			ret = talloc_asprintf(ctx, "%s\nDomain %u: %ld s",
+					      ret ? : "Domains with long running transactions:",
+					      conn->id, tdiff);
+		}
+	}
+
+	return ret ? (const char *)ret : "Overlapping transactions";
+}
+
+static const char *lu_dump_state(const void *ctx, struct connection *conn)
+{
+	FILE *fp;
+	const char *ret;
+	struct xs_state_record_header end;
+	struct xs_state_preamble pre;
+
+	fp = lu_dump_open(ctx);
+	if (!fp)
+		return "Dump state open error";
+
+	memcpy(pre.ident, XS_STATE_IDENT, sizeof(pre.ident));
+	pre.version = htobe32(XS_STATE_VERSION);
+	pre.flags = XS_STATE_FLAGS;
+	if (fwrite(&pre, sizeof(pre), 1, fp) != 1) {
+		ret = "Dump write error";
+		goto out;
+	}
+
+	ret = dump_state_global(fp);
+	if (ret)
+		goto out;
+	ret = dump_state_connections(fp);
+	if (ret)
+		goto out;
+	ret = dump_state_nodes(fp, ctx);
+	if (ret)
+		goto out;
+
+	end.type = XS_STATE_TYPE_END;
+	end.length = 0;
+	if (fwrite(&end, sizeof(end), 1, fp) != 1)
+		ret = "Dump write error";
+
+ out:
+	lu_dump_close(fp);
+
+	return ret;
+}
+
+static const char *lu_activate_binary(const void *ctx)
+{
+	int argc;
+	char **argv;
+	unsigned int i;
+
+	if (lu_status->cmdline) {
+		argc = 4;   /* At least one arg + progname + "-U" + NULL. */
+		for (i = 0; lu_status->cmdline[i]; i++)
+			if (isspace(lu_status->cmdline[i]))
+				argc++;
+		argv = talloc_array(ctx, char *, argc);
+		if (!argv)
+			return "Allocation failure.";
+
+		i = 0;
+		argc = 1;
+		argv[1] = strtok(lu_status->cmdline, " \t");
+		while (argv[argc]) {
+			if (!strcmp(argv[argc], "-U"))
+				i = 1;
+			argc++;
+			argv[argc] = strtok(NULL, " \t");
+		}
+
+		if (!i) {
+			argv[argc++] = "-U";
+			argv[argc] = NULL;
+		}
+	} else {
+		for (i = 0; i < orig_argc; i++)
+			if (!strcmp(orig_argv[i], "-U"))
+				break;
+
+		argc = orig_argc;
+		argv = talloc_array(ctx, char *, orig_argc + 2);
+		if (!argv)
+			return "Allocation failure.";
+
+		memcpy(argv, orig_argv, orig_argc * sizeof(*argv));
+		if (i == orig_argc)
+			argv[argc++] = "-U";
+		argv[argc] = NULL;
+	}
+
+	domain_deinit();
+
+	return lu_exec(ctx, argc, argv);
+}
+
+static bool do_lu_start(struct delayed_request *req)
+{
+	time_t now = time(NULL);
+	const char *ret;
+	struct buffered_data *saved_in;
+	struct connection *conn = req->data;
+
+	/*
+	 * Cancellation may have been requested asynchronously. In this
+	 * case, lu_status will be NULL.
+	 */
+	if (!lu_status) {
+		ret = "Cancellation was requested";
+		goto out;
+	}
+
+	assert(lu_status->conn == conn);
+
+	if (!lu_check_lu_allowed()) {
+		if (now < lu_status->started_at + lu_status->timeout)
+			return false;
+		if (!lu_status->force) {
+			ret = lu_reject_reason(req);
+			goto out;
+		}
+	}
+
+	assert(req->in == lu_status->in);
+	/* Dump out internal state, including "OK" for live update. */
+	ret = lu_dump_state(req->in, conn);
+	if (!ret) {
+		/* Perform the activation of new binary. */
+		ret = lu_activate_binary(req->in);
+	}
+
+	/* We will reach this point only in case of failure. */
+ out:
+	/*
+	 * send_reply() will send the response for conn->in. Save the current
+	 * conn->in and restore it afterwards.
+	 */
+	saved_in = conn->in;
+	conn->in = req->in;
+	send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
+	conn->in = saved_in;
+	talloc_free(lu_status);
+
+	return true;
+}
+
+static const char *lu_start(const void *ctx, struct connection *conn,
+			    bool force, unsigned int to)
+{
+	syslog(LOG_INFO, "live-update: start, force=%d, to=%u\n", force, to);
+
+	if (!lu_status || lu_status->conn != conn)
+		return "Not in live-update session.";
+
+#ifdef __MINIOS__
+	if (lu_status->kernel_size != lu_status->kernel_off)
+		return "Kernel not complete.";
+#endif
+
+	lu_status->force = force;
+	lu_status->timeout = to;
+	lu_status->started_at = time(NULL);
+	lu_status->in = conn->in;
+
+	errno = delay_request(conn, conn->in, do_lu_start, conn, false);
+
+	return NULL;
+}
+
+int do_control_lu(const void *ctx, struct connection *conn, char **vec,
+		  int num)
+{
+	const char *ret = NULL;
+	unsigned int i;
+	bool force = false;
+	unsigned int to = 0;
+
+	if (num < 1)
+		return EINVAL;
+
+	if (!strcmp(vec[0], "-a")) {
+		if (num == 1)
+			ret = lu_abort(ctx, conn);
+		else
+			return EINVAL;
+	} else if (!strcmp(vec[0], "-c")) {
+		if (num == 2)
+			ret = lu_cmdline(ctx, conn, vec[1]);
+		else
+			return EINVAL;
+	} else if (!strcmp(vec[0], "-s")) {
+		for (i = 1; i < num; i++) {
+			if (!strcmp(vec[i], "-F"))
+				force = true;
+			else if (!strcmp(vec[i], "-t") && i < num - 1) {
+				i++;
+				to = atoi(vec[i]);
+			} else
+				return EINVAL;
+		}
+		ret = lu_start(ctx, conn, force, to);
+		if (!ret)
+			return errno;
+	} else {
+		ret = lu_arch(ctx, conn, vec, num);
+		if (!ret && errno)
+			return errno;
+	}
+
+	if (!ret)
+		ret = "OK";
+	send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
+	return 0;
+}
+#endif
diff --git a/tools/xenstore/xenstored_lu.h b/tools/xenstore/xenstored_lu.h
index d2f8e4e57c..7aa524a07e 100644
--- a/tools/xenstore/xenstored_lu.h
+++ b/tools/xenstore/xenstored_lu.h
@@ -43,6 +43,16 @@ struct lu_dump_state {
 
 extern struct live_update *lu_status;
 
+struct connection *lu_get_connection(void);
+bool lu_is_pending(void);
+void lu_read_state(void);
+
+/* Write the "OK" response for the live-update command */
+unsigned int lu_write_response(FILE *fp);
+
+int do_control_lu(const void *ctx, struct connection *conn, char **vec,
+		  int num);
+
 /* Live update private interfaces. */
 void lu_get_dump_state(struct lu_dump_state *state);
 void lu_close_dump_state(struct lu_dump_state *state);
@@ -53,4 +63,19 @@ const char *lu_arch(const void *ctx, struct connection *conn, char **vec,
 		    int num);
 const char *lu_begin(struct connection *conn);
 int lu_destroy(void *data);
+#else
+static inline struct connection *lu_get_connection(void)
+{
+	return NULL;
+}
+
+static inline unsigned int lu_write_response(FILE *fp)
+{
+	return 0;
+}
+
+static inline bool lu_is_pending(void)
+{
+	return false;
+}
 #endif
-- 
2.35.3

[PATCH v3 16/16] tools/xenstore: remove unused stuff from list.h

[Juergen Gross]

Remove the hlist defines/functions and the rcu related functions from
tools/xenstore/list.h, as they are not used.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/list.h | 227 ------------------------------------------
 1 file changed, 227 deletions(-)

diff --git a/tools/xenstore/list.h b/tools/xenstore/list.h
index a464a38b61..d722a91220 100644
--- a/tools/xenstore/list.h
+++ b/tools/xenstore/list.h
@@ -88,48 +88,6 @@ static inline void list_add_tail(struct list_head *new, struct list_head *head)
 	__list_add(new, head->prev, head);
 }
 
-/*
- * Insert a new entry between two known consecutive entries. 
- *
- * This is only for internal list manipulation where we know
- * the prev/next entries already!
- */
-static __inline__ void __list_add_rcu(struct list_head * new,
-	struct list_head * prev,
-	struct list_head * next)
-{
-	new->next = next;
-	new->prev = prev;
-	next->prev = new;
-	prev->next = new;
-}
-
-/**
- * list_add_rcu - add a new entry to rcu-protected list
- * @new: new entry to be added
- * @head: list head to add it after
- *
- * Insert a new entry after the specified head.
- * This is good for implementing stacks.
- */
-static __inline__ void list_add_rcu(struct list_head *new, struct list_head *head)
-{
-	__list_add_rcu(new, head, head->next);
-}
-
-/**
- * list_add_tail_rcu - add a new entry to rcu-protected list
- * @new: new entry to be added
- * @head: list head to add it before
- *
- * Insert a new entry before the specified head.
- * This is useful for implementing queues.
- */
-static __inline__ void list_add_tail_rcu(struct list_head *new, struct list_head *head)
-{
-	__list_add_rcu(new, head->prev, head);
-}
-
 /*
  * Delete a list entry by making the prev/next entries
  * point to each other.
@@ -156,23 +114,6 @@ static inline void list_del(struct list_head *entry)
 	entry->prev = LIST_POISON2;
 }
 
-/**
- * list_del_rcu - deletes entry from list without re-initialization
- * @entry: the element to delete from the list.
- *
- * Note: list_empty on entry does not return true after this, 
- * the entry is in an undefined state. It is useful for RCU based
- * lockfree traversal.
- *
- * In particular, it means that we can not poison the forward 
- * pointers that may still be used for walking the list.
- */
-static inline void list_del_rcu(struct list_head *entry)
-{
-	__list_del(entry->prev, entry->next);
-	entry->prev = LIST_POISON2;
-}
-
 /**
  * list_del_init - deletes entry from list and reinitialize it.
  * @entry: the element to delete from the list.
@@ -339,172 +280,4 @@ static inline void list_splice_init(struct list_head *list,
 	     &pos->member != (head); 					\
 	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
 
-
-/* 
- * Double linked lists with a single pointer list head. 
- * Mostly useful for hash tables where the two pointer list head is 
- * too wasteful.
- * You lose the ability to access the tail in O(1).
- */ 
-
-struct hlist_head { 
-	struct hlist_node *first; 
-}; 
-
-struct hlist_node { 
-	struct hlist_node *next, **pprev; 
-}; 
-
-#define HLIST_HEAD_INIT { .first = NULL } 
-#define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
-#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL) 
-#define INIT_HLIST_NODE(ptr) ((ptr)->next = NULL, (ptr)->pprev = NULL)
-
-static __inline__ int hlist_unhashed(struct hlist_node *h) 
-{ 
-	return !h->pprev;
-} 
-
-static __inline__ int hlist_empty(struct hlist_head *h) 
-{ 
-	return !h->first;
-} 
-
-static __inline__ void __hlist_del(struct hlist_node *n) 
-{
-	struct hlist_node *next = n->next;
-	struct hlist_node **pprev = n->pprev;
-	*pprev = next;  
-	if (next) 
-		next->pprev = pprev;
-}  
-
-static __inline__ void hlist_del(struct hlist_node *n)
-{
-	__hlist_del(n);
-	n->next = LIST_POISON1;
-	n->pprev = LIST_POISON2;
-}
-
-/**
- * hlist_del_rcu - deletes entry from hash list without re-initialization
- * @entry: the element to delete from the hash list.
- *
- * Note: list_unhashed() on entry does not return true after this, 
- * the entry is in an undefined state. It is useful for RCU based
- * lockfree traversal.
- *
- * In particular, it means that we can not poison the forward
- * pointers that may still be used for walking the hash list.
- */
-static inline void hlist_del_rcu(struct hlist_node *n)
-{
-	__hlist_del(n);
-	n->pprev = LIST_POISON2;
-}
-
-static __inline__ void hlist_del_init(struct hlist_node *n) 
-{
-	if (n->pprev)  {
-		__hlist_del(n);
-		INIT_HLIST_NODE(n);
-	}
-}  
-
-#define hlist_del_rcu_init hlist_del_init
-
-static __inline__ void hlist_add_head(struct hlist_node *n, struct hlist_head *h) 
-{ 
-	struct hlist_node *first = h->first;
-	n->next = first; 
-	if (first) 
-		first->pprev = &n->next;
-	h->first = n; 
-	n->pprev = &h->first; 
-} 
-
-static __inline__ void hlist_add_head_rcu(struct hlist_node *n, struct hlist_head *h) 
-{ 
-	struct hlist_node *first = h->first;
-	n->next = first;
-	n->pprev = &h->first; 
-	if (first) 
-		first->pprev = &n->next;
-	h->first = n; 
-} 
-
-/* next must be != NULL */
-static __inline__ void hlist_add_before(struct hlist_node *n, struct hlist_node *next)
-{
-	n->pprev = next->pprev;
-	n->next = next; 
-	next->pprev = &n->next; 
-	*(n->pprev) = n;
-}
-
-static __inline__ void hlist_add_after(struct hlist_node *n,
-				       struct hlist_node *next)
-{
-	next->next	= n->next;
-	*(next->pprev)	= n;
-	n->next		= next;
-}
-
-#define hlist_entry(ptr, type, member) container_of(ptr,type,member)
-
-/* Cannot easily do prefetch unfortunately */
-#define hlist_for_each(pos, head) \
-	for (pos = (head)->first; pos; pos = pos->next) 
-
-#define hlist_for_each_safe(pos, n, head) \
-	for (pos = (head)->first; n = pos ? pos->next : 0, pos; \
-	     pos = n)
-
-/**
- * hlist_for_each_entry	- iterate over list of given type
- * @tpos:	the type * to use as a loop counter.
- * @pos:	the &struct hlist_node to use as a loop counter.
- * @head:	the head for your list.
- * @member:	the name of the hlist_node within the struct.
- */
-#define hlist_for_each_entry(tpos, pos, head, member)			 \
-	for (pos = (head)->first;					 \
-	     pos && ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
-
-/**
- * hlist_for_each_entry_continue - iterate over a hlist continuing after existing point
- * @tpos:	the type * to use as a loop counter.
- * @pos:	the &struct hlist_node to use as a loop counter.
- * @member:	the name of the hlist_node within the struct.
- */
-#define hlist_for_each_entry_continue(tpos, pos, member)		 \
-	for (pos = (pos)->next;						 \
-	     pos && ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
-
-/**
- * hlist_for_each_entry_from - iterate over a hlist continuing from existing point
- * @tpos:	the type * to use as a loop counter.
- * @pos:	the &struct hlist_node to use as a loop counter.
- * @member:	the name of the hlist_node within the struct.
- */
-#define hlist_for_each_entry_from(tpos, pos, member)			 \
-	for (; pos && ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
-
-/**
- * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
- * @tpos:	the type * to use as a loop counter.
- * @pos:	the &struct hlist_node to use as a loop counter.
- * @n:		another &struct hlist_node to use as temporary storage
- * @head:	the head for your list.
- * @member:	the name of the hlist_node within the struct.
- */
-#define hlist_for_each_entry_safe(tpos, pos, n, head, member) 		 \
-	for (pos = (head)->first;					 \
-	     pos && ({ n = pos->next; 1; }) && 				 \
-		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = n)
-
 #endif
-- 
2.35.3

Re: [PATCH v3 03/16] tools/xenstore: modify interface of create_hashtable()

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> The minsize parameter of create_hashtable() doesn't have any real use
> case for Xenstore, so drop it.
> 
> For better talloc_report_full() diagnostic output add a name parameter
> to create_hashtable().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 04/16] tools/xenstore: rename hashtable_insert() and let it return 0 on success

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Today hashtable_insert() returns 0 in case of an error. Change that to
> let it return an errno value in the error case and 0 in case of success.
> In order to avoid any missed return value checks or related future
> backport errors, rename hashtable_insert() to hashtable_add().
> 
> Even if not used today, do the same switch for the return value of
> hashtable_expand().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 05/16] tools/xenstore: make some write limit functions static

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Some wrl_*() functions are only used in xenstored_domain.c, so make
> them static. In order to avoid the need of forward declarations, move
> the whole function block to the start of the file.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 06/16] tools/xenstore: switch write limiting to use millisecond time base

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> There is no need to keep struct wrl_timestampt, as it serves the same
> purpose as the more simple time base provided by get_now().
> 
> Move some more stuff from xenstored_domain.h into xenstored_domain.c
> as it is being used nowhere else.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 09/16] tools/xenstore: remove support of file backed data base

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> In order to prepare the replacement of TDB with direct accessible nodes
> in memory, remove the support for a file backed data base.
> 
> This allows to remove xs_tdb_dump, too.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Replace the usage of the xenstore private list.h header with the
> common xen_list.h one.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3:
> - new patch
> ---
>   tools/libs/store/xs.c | 56 +++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 7a9a8b1656..3813b69ae2 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -35,13 +35,13 @@
>   #include <errno.h>
>   #include "xenstore.h"
>   #include "xs_lib.h"
> -#include "list.h"
>   #include "utils.h"
>   
>   #include <xentoolcore_internal.h>
> +#include <xen_list.h>
>   
>   struct xs_stored_msg {
> -	struct list_head list;
> +	XEN_TAILQ_ENTRY(struct xs_stored_msg) list;

I have expected us to use to XEN_LIST_*. Can you explain why you didn't 
use them?

>   	struct xsd_sockmsg hdr;
>   	char *body;
>   };
> @@ -70,7 +70,7 @@ struct xs_handle {
>            * A list of fired watch messages, protected by a mutex. Users can
>            * wait on the conditional variable until a watch is pending.
>            */
> -	struct list_head watch_list;
> +	XEN_TAILQ_HEAD(, struct xs_stored_msg) watch_list;
>   	pthread_mutex_t watch_mutex;
>   	pthread_cond_t watch_condvar;
>   
> @@ -84,7 +84,7 @@ struct xs_handle {
>            * because we serialise requests. The requester can wait on the
>            * conditional variable for its response.
>            */
> -	struct list_head reply_list;
> +	XEN_TAILQ_HEAD(, struct xs_stored_msg) reply_list;
>   	pthread_mutex_t reply_mutex;
>   	pthread_cond_t reply_condvar;
>   
> @@ -133,8 +133,8 @@ static void *read_thread(void *arg);
>   struct xs_handle {
>   	int fd;
>   	Xentoolcore__Active_Handle tc_ah; /* for restrict */
> -	struct list_head reply_list;
> -	struct list_head watch_list;
> +	XEN_TAILQ_HEAD(, struct xs_stored_msg) reply_list;
> +	XEN_TAILQ_HEAD(, struct xs_stored_msg) watch_list;
>   	/* Clients can select() on this pipe to wait for a watch to fire. */
>   	int watch_pipe[2];
>   	/* Filtering watch event in unwatch function? */
> @@ -180,7 +180,7 @@ int xs_fileno(struct xs_handle *h)
>   
>   	if ((h->watch_pipe[0] == -1) && (pipe(h->watch_pipe) != -1)) {
>   		/* Kick things off if the watch list is already non-empty. */
> -		if (!list_empty(&h->watch_list))
> +		if (!XEN_TAILQ_EMPTY(&h->watch_list))
>   			while (write(h->watch_pipe[1], &c, 1) != 1)
>   				continue;
>   	}
> @@ -262,8 +262,8 @@ static struct xs_handle *get_handle(const char *connect_to)
>   	if (h->fd == -1)
>   		goto err;
>   
> -	INIT_LIST_HEAD(&h->reply_list);
> -	INIT_LIST_HEAD(&h->watch_list);
> +	XEN_TAILQ_INIT(&h->reply_list);
> +	XEN_TAILQ_INIT(&h->watch_list);
>   
>   	/* Watch pipe is allocated on demand in xs_fileno(). */
>   	h->watch_pipe[0] = h->watch_pipe[1] = -1;
> @@ -329,12 +329,12 @@ struct xs_handle *xs_open(unsigned long flags)
>   static void close_free_msgs(struct xs_handle *h) {
>   	struct xs_stored_msg *msg, *tmsg;
>   
> -	list_for_each_entry_safe(msg, tmsg, &h->reply_list, list) {
> +	XEN_TAILQ_FOREACH_SAFE(msg, &h->reply_list, list, tmsg) {
>   		free(msg->body);
>   		free(msg);
>   	}
>   
> -	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
> +	XEN_TAILQ_FOREACH_SAFE(msg, &h->watch_list, list, tmsg) {
>   		free(msg->body);
>   		free(msg);
>   	}
> @@ -459,17 +459,17 @@ static void *read_reply(
>   
>   	mutex_lock(&h->reply_mutex);
>   #ifdef USE_PTHREAD
> -	while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
> +	while (XEN_TAILQ_EMPTY(&h->reply_list) && read_from_thread && h->fd != -1)
>   		condvar_wait(&h->reply_condvar, &h->reply_mutex);
>   #endif
> -	if (list_empty(&h->reply_list)) {
> +	if (XEN_TAILQ_EMPTY(&h->reply_list)) {
>   		mutex_unlock(&h->reply_mutex);
>   		errno = EINVAL;
>   		return NULL;
>   	}
> -	msg = list_top(&h->reply_list, struct xs_stored_msg, list);
> -	list_del(&msg->list);
> -	assert(list_empty(&h->reply_list));
> +	msg = XEN_TAILQ_FIRST(&h->reply_list);
> +	XEN_TAILQ_REMOVE(&h->reply_list, msg, list);
> +	assert(XEN_TAILQ_EMPTY(&h->reply_list));
>   	mutex_unlock(&h->reply_mutex);
>   
>   	*type = msg->hdr.type;
> @@ -883,7 +883,7 @@ static void xs_maybe_clear_watch_pipe(struct xs_handle *h)
>   {
>   	char c;
>   
> -	if (list_empty(&h->watch_list) && (h->watch_pipe[0] != -1))
> +	if (XEN_TAILQ_EMPTY(&h->watch_list) && (h->watch_pipe[0] != -1))
>   		while (read(h->watch_pipe[0], &c, 1) != 1)
>   			continue;
>   }
> @@ -907,7 +907,7 @@ static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
>   	 * we haven't called xs_watch.	Presumably the application
>   	 * will do so later; in the meantime we just block.
>   	 */
> -	while (list_empty(&h->watch_list) && h->fd != -1) {
> +	while (XEN_TAILQ_EMPTY(&h->watch_list) && h->fd != -1) {
>   		if (nonblocking) {
>   			mutex_unlock(&h->watch_mutex);
>   			errno = EAGAIN;
> @@ -925,13 +925,13 @@ static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
>   
>   #endif /* !defined(USE_PTHREAD) */
>   
> -	if (list_empty(&h->watch_list)) {
> +	if (XEN_TAILQ_EMPTY(&h->watch_list)) {
>   		mutex_unlock(&h->watch_mutex);
>   		errno = EINVAL;
>   		return NULL;
>   	}
> -	msg = list_top(&h->watch_list, struct xs_stored_msg, list);
> -	list_del(&msg->list);
> +	msg = XEN_TAILQ_FIRST(&h->watch_list);
> +	XEN_TAILQ_REMOVE(&h->watch_list, msg, list);
>   
>   	xs_maybe_clear_watch_pipe(h);
>   	mutex_unlock(&h->watch_mutex);
> @@ -1007,12 +1007,12 @@ bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
>   	/* Filter the watch list to remove potential message */
>   	mutex_lock(&h->watch_mutex);
>   
> -	if (list_empty(&h->watch_list)) {
> +	if (XEN_TAILQ_EMPTY(&h->watch_list)) {
>   		mutex_unlock(&h->watch_mutex);
>   		return res;
>   	}
>   
> -	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
> +	XEN_TAILQ_FOREACH_SAFE(msg, &h->watch_list, list, tmsg) {
>   		assert(msg->hdr.type == XS_WATCH_EVENT);
>   
>   		s = msg->body;
> @@ -1034,7 +1034,7 @@ bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
>   
>   		if (l_token && !strcmp(token, l_token) &&
>   		    l_path && xs_path_is_subpath(path, l_path)) {
> -			list_del(&msg->list);
> +			XEN_TAILQ_REMOVE(&h->watch_list, msg, list);
>   			free(msg);
>   		}
>   	}
> @@ -1290,12 +1290,12 @@ static int read_message(struct xs_handle *h, int nonblocking)
>   		cleanup_push(pthread_mutex_unlock, &h->watch_mutex);
>   
>   		/* Kick users out of their select() loop. */
> -		if (list_empty(&h->watch_list) &&
> +		if (XEN_TAILQ_EMPTY(&h->watch_list) &&
>   		    (h->watch_pipe[1] != -1))
>   			while (write(h->watch_pipe[1], body, 1) != 1) /* Cancellation point */
>   				continue;
>   
> -		list_add_tail(&msg->list, &h->watch_list);
> +		XEN_TAILQ_INSERT_TAIL(&h->watch_list, msg, list);
>   
>   		condvar_signal(&h->watch_condvar);
>   
> @@ -1304,13 +1304,13 @@ static int read_message(struct xs_handle *h, int nonblocking)
>   		mutex_lock(&h->reply_mutex);
>   
>   		/* There should only ever be one response pending! */
> -		if (!list_empty(&h->reply_list)) {
> +		if (!XEN_TAILQ_EMPTY(&h->reply_list)) {
>   			mutex_unlock(&h->reply_mutex);
>   			saved_errno = EEXIST;
>   			goto error_freebody;
>   		}
>   
> -		list_add_tail(&msg->list, &h->reply_list);
> +		XEN_TAILQ_INSERT_TAIL(&h->reply_list, msg, list);
>   		condvar_signal(&h->reply_condvar);
>   
>   		mutex_unlock(&h->reply_mutex);

-- 
Julien Grall

Re: [PATCH v3 11/16] tools/libs/store: make libxenstore independent of utils.h

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> There is no real need for including tools/xenstore/utils.h from
> libxenstore, as only streq() and ARRAY_SIZE() are obtained via that
> header.
> 
> streq() is just !strcmp(), and ARRAY_SIZE() is brought in via
> xen-tools/common-macros.h.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 00/16] tools/xenstore: more cleanups

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Some more cleanups of Xenstore.
> 
> Based on top of the previous Xenstore series "tools/xenstore: rework
> internal accounting".
> 
> Changes in V2:
> - rebase
> - one small modification of patch 10
> - added patches 11-13
> 
> Changes in V3:
> - rebase
> - modified patch 4
> - added patches 10, 11 and 13
> 
> Juergen Gross (16):
>    tools/xenstore: verify command line parameters better
>    tools/xenstore: do some cleanup of hashtable.c
>    tools/xenstore: modify interface of create_hashtable()
>    tools/xenstore: rename hashtable_insert() and let it return 0 on
>      success
>    tools/xenstore: make some write limit functions static
>    tools/xenstore: switch write limiting to use millisecond time base
>    tools/xenstore: remove stale TODO file
>    tools/xenstore: remove unused events list
>    tools/xenstore: remove support of file backed data base

I have committed up to here. I still need to review the rest.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1108 bytes --]

On 09.06.23 20:09, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/05/2023 09:54, Juergen Gross wrote:
>> Replace the usage of the xenstore private list.h header with the
>> common xen_list.h one.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3:
>> - new patch
>> ---
>>   tools/libs/store/xs.c | 56 +++++++++++++++++++++----------------------
>>   1 file changed, 28 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>> index 7a9a8b1656..3813b69ae2 100644
>> --- a/tools/libs/store/xs.c
>> +++ b/tools/libs/store/xs.c
>> @@ -35,13 +35,13 @@
>>   #include <errno.h>
>>   #include "xenstore.h"
>>   #include "xs_lib.h"
>> -#include "list.h"
>>   #include "utils.h"
>>   #include <xentoolcore_internal.h>
>> +#include <xen_list.h>
>>   struct xs_stored_msg {
>> -    struct list_head list;
>> +    XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
> 
> I have expected us to use to XEN_LIST_*. Can you explain why you didn't use them?

XEN_LIST_* doesn't provide a list_add_tail() replacement.


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

Re: [PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Julien Grall]

Hi,

On 12/06/2023 08:02, Juergen Gross wrote:
> On 09.06.23 20:09, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 30/05/2023 09:54, Juergen Gross wrote:
>>> Replace the usage of the xenstore private list.h header with the
>>> common xen_list.h one.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V3:
>>> - new patch
>>> ---
>>>   tools/libs/store/xs.c | 56 +++++++++++++++++++++----------------------
>>>   1 file changed, 28 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>> index 7a9a8b1656..3813b69ae2 100644
>>> --- a/tools/libs/store/xs.c
>>> +++ b/tools/libs/store/xs.c
>>> @@ -35,13 +35,13 @@
>>>   #include <errno.h>
>>>   #include "xenstore.h"
>>>   #include "xs_lib.h"
>>> -#include "list.h"
>>>   #include "utils.h"
>>>   #include <xentoolcore_internal.h>
>>> +#include <xen_list.h>
>>>   struct xs_stored_msg {
>>> -    struct list_head list;
>>> +    XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
>>
>> I have expected us to use to XEN_LIST_*. Can you explain why you 
>> didn't use them?
> 
> XEN_LIST_* doesn't provide a list_add_tail() replacement.

Ok. Did you look at whether list_add_tail() could be replaced with 
adding the element at the head?

Anyway, I can understand that this would not be a straight swap. But it 
would be good to have some rationale in the commit message as this helps 
understanding the choice.

With that:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1803 bytes --]

On 12.06.23 12:34, Julien Grall wrote:
> Hi,
> 
> On 12/06/2023 08:02, Juergen Gross wrote:
>> On 09.06.23 20:09, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 30/05/2023 09:54, Juergen Gross wrote:
>>>> Replace the usage of the xenstore private list.h header with the
>>>> common xen_list.h one.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V3:
>>>> - new patch
>>>> ---
>>>>   tools/libs/store/xs.c | 56 +++++++++++++++++++++----------------------
>>>>   1 file changed, 28 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>>> index 7a9a8b1656..3813b69ae2 100644
>>>> --- a/tools/libs/store/xs.c
>>>> +++ b/tools/libs/store/xs.c
>>>> @@ -35,13 +35,13 @@
>>>>   #include <errno.h>
>>>>   #include "xenstore.h"
>>>>   #include "xs_lib.h"
>>>> -#include "list.h"
>>>>   #include "utils.h"
>>>>   #include <xentoolcore_internal.h>
>>>> +#include <xen_list.h>
>>>>   struct xs_stored_msg {
>>>> -    struct list_head list;
>>>> +    XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
>>>
>>> I have expected us to use to XEN_LIST_*. Can you explain why you didn't use 
>>> them?
>>
>> XEN_LIST_* doesn't provide a list_add_tail() replacement.
> 
> Ok. Did you look at whether list_add_tail() could be replaced with adding the 
> element at the head?
> 
> Anyway, I can understand that this would not be a straight swap. But it would be 
> good to have some rationale in the commit message as this helps understanding 
> the choice.

Okay, I'll add: "Use the XEN_TAILQ type list, as it allows to directly swap the
related macros/functions without having to change the logic."

> 
> With that:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks,


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

Re: [PATCH v3 10/16] tools/libs/store: use xen_list.h instead of xenstore/list.h

[Julien Grall]

On 12/06/2023 11:37, Juergen Gross wrote:
> On 12.06.23 12:34, Julien Grall wrote:
>> Hi,
>>
>> On 12/06/2023 08:02, Juergen Gross wrote:
>>> On 09.06.23 20:09, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 30/05/2023 09:54, Juergen Gross wrote:
>>>>> Replace the usage of the xenstore private list.h header with the
>>>>> common xen_list.h one.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> V3:
>>>>> - new patch
>>>>> ---
>>>>>   tools/libs/store/xs.c | 56 
>>>>> +++++++++++++++++++++----------------------
>>>>>   1 file changed, 28 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>>>> index 7a9a8b1656..3813b69ae2 100644
>>>>> --- a/tools/libs/store/xs.c
>>>>> +++ b/tools/libs/store/xs.c
>>>>> @@ -35,13 +35,13 @@
>>>>>   #include <errno.h>
>>>>>   #include "xenstore.h"
>>>>>   #include "xs_lib.h"
>>>>> -#include "list.h"
>>>>>   #include "utils.h"
>>>>>   #include <xentoolcore_internal.h>
>>>>> +#include <xen_list.h>
>>>>>   struct xs_stored_msg {
>>>>> -    struct list_head list;
>>>>> +    XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
>>>>
>>>> I have expected us to use to XEN_LIST_*. Can you explain why you 
>>>> didn't use them?
>>>
>>> XEN_LIST_* doesn't provide a list_add_tail() replacement.
>>
>> Ok. Did you look at whether list_add_tail() could be replaced with 
>> adding the element at the head?
>>
>> Anyway, I can understand that this would not be a straight swap. But 
>> it would be good to have some rationale in the commit message as this 
>> helps understanding the choice.
> 
> Okay, I'll add: "Use the XEN_TAILQ type list, as it allows to directly 
> swap the
> related macros/functions without having to change the logic."

Thanks. I am happy to add it whilst committing.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 12/16] tools/xenstore: remove no longer needed functions from xs_lib.c

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> xs_daemon_tdb() in xs_lib.c is no longer used at all, so it can be
> removed. xs_domain_dev() and xs_write_all() are not used by xenstored,
> so they can be moved to tools/libs/store/xs.c.
> 
> xs_daemon_rootdir() is used by xenstored only and it only calls
> xs_daemon_rundir(), so replace its use cases with xs_daemon_rundir()
> and remove it from xs_lib.c.
> 
> xs_daemon_socket_ro() is needed in libxenstore only, so move it to
> tools/libs/store/xs.c.
> 
> Move functions used by xenstore-client only to xenstore_client.c.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 13/16] tools/xenstore: replace xs_lib.c with a header

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Instead of including the same small C source in multiple binaries from
> 2 source directories, use a header file with inline functions as a
> replacement.
> 
> As some of the functions are exported by libxenstore, rename the inline
> functions from xs_*() do xenstore_*() and add xs_*() wrappers to
> libxenstore.
> 
> With that no sources required to build libxenstore are left in
> tools/xenstore, so the file COPYING can be removed now.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 14/16] tools/xenstore: split out environment specific live update code

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> -static struct live_update *lu_status;
> -
> -struct lu_dump_state {
> -	void *buf;
> -	unsigned int size;
> -#ifndef __MINIOS__
> -	int fd;
> -	char *filename;
> -#endif
> -};
> -
> -static int lu_destroy(void *data)
> -{
> -#ifdef __MINIOS__
> -	if (lu_status->dump_state)
> -		munmap(lu_status->dump_state, lu_status->dump_size);
> -#endif
> -	lu_status = NULL;
> -
> -	return 0;
> -}

I think moving all lu_destroy() out of xenstored_control.c is a mistake 
because we now need to remember that any common change in lu_begin() may 
need an update in the two implementation of lu_destroy().

Even if this seems pointless for a few lines, it would be best to split 
lu_destroy() in two parts: one common and one per lu backend.

The rest of the changes look good to me.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 15/16] tools/xenstore: split out rest of live update control code

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Move the rest of live update related code from xenstored_control.c to
> a dedicated new source file.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Depending on the outcome of the discussion in patch #15, this patch may 
need to be updated. At its current state:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 16/16] tools/xenstore: remove unused stuff from list.h

[Julien Grall]

Hi Juergen,

On 30/05/2023 09:54, Juergen Gross wrote:
> Remove the hlist defines/functions and the rcu related functions from
> tools/xenstore/list.h, as they are not used.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 14/16] tools/xenstore: split out environment specific live update code

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1049 bytes --]

On 19.06.23 19:55, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/05/2023 09:54, Juergen Gross wrote:
>> -static struct live_update *lu_status;
>> -
>> -struct lu_dump_state {
>> -    void *buf;
>> -    unsigned int size;
>> -#ifndef __MINIOS__
>> -    int fd;
>> -    char *filename;
>> -#endif
>> -};
>> -
>> -static int lu_destroy(void *data)
>> -{
>> -#ifdef __MINIOS__
>> -    if (lu_status->dump_state)
>> -        munmap(lu_status->dump_state, lu_status->dump_size);
>> -#endif
>> -    lu_status = NULL;
>> -
>> -    return 0;
>> -}
> 
> I think moving all lu_destroy() out of xenstored_control.c is a mistake because 
> we now need to remember that any common change in lu_begin() may need an update 
> in the two implementation of lu_destroy().
> 
> Even if this seems pointless for a few lines, it would be best to split 
> lu_destroy() in two parts: one common and one per lu backend.

Okay, fine with me.

> 
> The rest of the changes look good to me.

Thanks,

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