There are some internals in the libxenstore interface which should be
removed.

Move those functions into xs_lib.c and the related definitions into
xs_lib.h. Remove the functions from the mapfile. Add xs_lib.o to
xenstore_client as some of the internal functions are needed there.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/include/xenstore_lib.h       |  34 ---------
 tools/libs/store/libxenstore.map   |   8 --
 tools/libs/store/xs.c              | 112 +---------------------------
 tools/xenstore/Makefile            |   4 +-
 tools/xenstore/utils.h             |  11 +++
 tools/xenstore/xenstore_client.c   |   2 +
 tools/xenstore/xenstored_control.c |   1 +
 tools/xenstore/xenstored_core.c    |   2 +-
 tools/xenstore/xs_lib.c            | 114 ++++++++++++++++++++++++++++-
 tools/xenstore/xs_lib.h            |  50 +++++++++++++
 10 files changed, 181 insertions(+), 157 deletions(-)
 create mode 100644 tools/xenstore/xs_lib.h

diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
index 4c9b6d1685..a10465fa69 100644
--- a/tools/include/xenstore_lib.h
+++ b/tools/include/xenstore_lib.h
@@ -43,25 +43,13 @@ struct xs_permissions
 	enum xs_perm_type perms;
 };
 
-/* Header of the node record in tdb. */
-struct xs_tdb_record_hdr {
-	uint64_t generation;
-	uint32_t num_perms;
-	uint32_t datalen;
-	uint32_t childlen;
-	struct xs_permissions perms[0];
-};
-
 /* Each 10 bits takes ~ 3 digits, plus one, plus one for nul terminator. */
 #define MAX_STRLEN(x) ((sizeof(x) * CHAR_BIT + CHAR_BIT-1) / 10 * 3 + 2)
 
 /* Path for various daemon things: env vars can override. */
-const char *xs_daemon_rootdir(void);
 const char *xs_daemon_rundir(void);
 const char *xs_daemon_socket(void);
 const char *xs_daemon_socket_ro(void);
-const char *xs_domain_dev(void);
-const char *xs_daemon_tdb(void);
 
 /* Simple write function: loops for you. */
 bool xs_write_all(int fd, const void *data, unsigned int len);
@@ -70,26 +58,4 @@ bool xs_write_all(int fd, const void *data, unsigned int len);
 bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
 			 const char *strings);
 
-/* 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);
-
-/* Sanitising (quoting) possibly-binary strings. */
-struct expanding_buffer {
-	char *buf;
-	int avail;
-};
-
-/* Ensure that given expanding buffer has at least min_avail characters. */
-char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
-
-/* sanitise_value() may return NULL if malloc fails. */
-char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
-
-/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
-
 #endif /* XENSTORE_LIB_H */
diff --git a/tools/libs/store/libxenstore.map b/tools/libs/store/libxenstore.map
index 9854305a2c..44b0217f4c 100644
--- a/tools/libs/store/libxenstore.map
+++ b/tools/libs/store/libxenstore.map
@@ -32,18 +32,10 @@ VERS_3.0.3 {
 		xs_control_command;
 		xs_debug_command;
 		xs_suspend_evtchn_port;
-		xs_daemon_rootdir;
 		xs_daemon_rundir;
 		xs_daemon_socket;
 		xs_daemon_socket_ro;
-		xs_domain_dev;
-		xs_daemon_tdb;
 		xs_write_all;
 		xs_strings_to_perms;
-		xs_perm_to_string;
-		xs_count_strings;
-		expanding_buffer_ensure;
-		sanitise_value;
-		unsanitise_value;
 	local: *; /* Do not expose anything by default */
 };
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index c91377c27f..7a9a8b1656 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -34,6 +34,7 @@
 #include <stdint.h>
 #include <errno.h>
 #include "xenstore.h"
+#include "xs_lib.h"
 #include "list.h"
 #include "utils.h"
 
@@ -1358,117 +1359,6 @@ static void *read_thread(void *arg)
 }
 #endif
 
-char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
-{
-	int want;
-	char *got;
-
-	if (ebuf->avail >= min_avail)
-		return ebuf->buf;
-
-	if (min_avail >= INT_MAX/3)
-		return 0;
-
-	want = ebuf->avail + min_avail + 10;
-	got = realloc(ebuf->buf, want);
-	if (!got)
-		return 0;
-
-	ebuf->buf = got;
-	ebuf->avail = want;
-	return ebuf->buf;
-}
-
-char *sanitise_value(struct expanding_buffer *ebuf,
-		     const char *val, unsigned len)
-{
-	int used, remain, c;
-	unsigned char *ip;
-
-#define ADD(c) (ebuf->buf[used++] = (c))
-#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
-
-	assert(len < INT_MAX/5);
-
-	ip = (unsigned char *)val;
-	used = 0;
-	remain = len;
-
-	if (!expanding_buffer_ensure(ebuf, remain + 1))
-		return NULL;
-
-	while (remain-- > 0) {
-		c= *ip++;
-
-		if (c >= ' ' && c <= '~' && c != '\\') {
-			ADD(c);
-			continue;
-		}
-
-		if (!expanding_buffer_ensure(ebuf, used + remain + 5))
-			/* for "<used>\\nnn<remain>\0" */
-			return 0;
-
-		ADD('\\');
-		switch (c) {
-		case '\t':  ADD('t');   break;
-		case '\n':  ADD('n');   break;
-		case '\r':  ADD('r');   break;
-		case '\\':  ADD('\\');  break;
-		default:
-			if (c < 010) ADDF("%03o", c);
-			else         ADDF("x%02x", c);
-		}
-	}
-
-	ADD(0);
-	assert(used <= ebuf->avail);
-	return ebuf->buf;
-
-#undef ADD
-#undef ADDF
-}
-
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
-{
-	const char *ip;
-	char *op;
-	unsigned c;
-	int n;
-
-	for (ip = in, op = out; (c = *ip++); *op++ = c) {
-		if (c == '\\') {
-			c = *ip++;
-
-#define GETF(f) do {					\
-			n = 0;				\
-			sscanf(ip, f "%n", &c, &n);	\
-			ip += n;			\
-		} while (0)
-
-			switch (c) {
-			case 't':              c= '\t';            break;
-			case 'n':              c= '\n';            break;
-			case 'r':              c= '\r';            break;
-			case '\\':             c= '\\';            break;
-			case 'x':                    GETF("%2x");  break;
-			case '0': case '4':
-			case '1': case '5':
-			case '2': case '6':
-			case '3': case '7':    --ip; GETF("%3o");  break;
-			case 0:                --ip;               break;
-			default:;
-			}
-#undef GETF
-		}
-	}
-
-	*op = 0;
-
-	if (out_len_r)
-		*out_len_r = op - out;
-}
-
 /*
  * Local variables:
  *  mode: C
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index ab89e22d3a..01c9ccc70f 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -78,8 +78,8 @@ xenstored.a: $(XENSTORED_OBJS)
 $(CLIENTS): xenstore
 	ln -f xenstore $@
 
-xenstore: xenstore_client.o
-	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
+xenstore: xenstore_client.o xs_lib.o
+	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
 
 xenstore-control: xenstore_control.o
 	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
index 87713a8e5d..9d012b97c1 100644
--- a/tools/xenstore/utils.h
+++ b/tools/xenstore/utils.h
@@ -7,6 +7,17 @@
 
 #include <xen-tools/libs.h>
 
+#include "xenstore_lib.h"
+
+/* Header of the node record in tdb. */
+struct xs_tdb_record_hdr {
+	uint64_t generation;
+	uint32_t num_perms;
+	uint32_t datalen;
+	uint32_t childlen;
+	struct xs_permissions perms[0];
+};
+
 /* Is A == B ? */
 #define streq(a,b) (strcmp((a),(b)) == 0)
 
diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 8015bfe5be..150c03769a 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -22,6 +22,8 @@
 
 #include <sys/ioctl.h>
 
+#include "xs_lib.h"
+
 #define PATH_SEP '/'
 #define MAX_PATH_LEN 256
 
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 8e470f2b20..8569c97f99 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -34,6 +34,7 @@ Interactive commands for Xen Store Daemon.
 
 #include "utils.h"
 #include "talloc.h"
+#include "xs_lib.h"
 #include "xenstored_core.h"
 #include "xenstored_control.h"
 #include "xenstored_domain.h"
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 8033c1e0eb..01906d9f2a 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -46,7 +46,7 @@
 #include "utils.h"
 #include "list.h"
 #include "talloc.h"
-#include "xenstore_lib.h"
+#include "xs_lib.h"
 #include "xenstored_core.h"
 #include "xenstored_watch.h"
 #include "xenstored_transaction.h"
diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index 80c03acbea..10fa4c3ad0 100644
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -16,12 +16,13 @@
     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 "xenstore_lib.h"
+#include "xs_lib.h"
 
 /* Common routines for the Xen store daemon and client library. */
 
@@ -179,3 +180,114 @@ 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
new file mode 100644
index 0000000000..efa05997d6
--- /dev/null
+++ b/tools/xenstore/xs_lib.h
@@ -0,0 +1,50 @@
+/*
+    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"
+
+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);
+
+/* 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.26.2

Juergen Gross writes ("[PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface"):
> There are some internals in the libxenstore interface which should be
> removed.
> 
> Move those functions into xs_lib.c and the related definitions into
> xs_lib.h. Remove the functions from the mapfile. Add xs_lib.o to
> xenstore_client as some of the internal functions are needed there.

This seems wider in scope than I was expecting.

Reviewing it again makes me think that there are more concers than I
anticipated and I am now doubtful whether I want to take it in 4.15.


I thought at this stage we were just going to fix the
accidentally-exported symbols with improperly namespaced names.  It is
those for which I think that withdrawing them without an ABI soname
bump, in contravention of usual library ABI stability rules, will not
cause trouble in pracice.

My current thoughts are that several of these really ought not to be
withdrawn as they might cause actual trouble:

>  /* Path for various daemon things: env vars can override. */
> -const char *xs_daemon_rootdir(void);
> -const char *xs_domain_dev(void);
> -const char *xs_daemon_tdb(void);

Someone who was writing bindings might have exposed these without
knowing what they were, resulting in linkage to these symbols.

>  bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
>  			 const char *strings);
>  
> -/* 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);

Isn't this function potentially useful ?  It seems funny to have only
one of the conversion directions.

> +void unsanitise_value(char *out, unsigned *out_len_r, const char *in)

Is it possible to do sort this out in a more minimal way ?  Eg we
could change the name to namespace it properly.  (I haven't looked at
the code in detail and am still rather under-caffeinated so maybe I am
talking nonsense here.)

Ian.

On 24.03.21 12:02, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface"):
>> There are some internals in the libxenstore interface which should be
>> removed.
>>
>> Move those functions into xs_lib.c and the related definitions into
>> xs_lib.h. Remove the functions from the mapfile. Add xs_lib.o to
>> xenstore_client as some of the internal functions are needed there.
> 
> This seems wider in scope than I was expecting.
> 
> Reviewing it again makes me think that there are more concers than I
> anticipated and I am now doubtful whether I want to take it in 4.15.

I'm fine with that. TBH I would have been surprised if you'd just take
it. :-)

> I thought at this stage we were just going to fix the
> accidentally-exported symbols with improperly namespaced names.  It is
> those for which I think that withdrawing them without an ABI soname
> bump, in contravention of usual library ABI stability rules, will not
> cause trouble in pracice.

Just removing them from the mapfile doesn't work.

Either we need to keep them (maybe with "xs_" prefixed), or we need
to go the way I've done in this patch.

> My current thoughts are that several of these really ought not to be
> withdrawn as they might cause actual trouble:
> 
>>   /* Path for various daemon things: env vars can override. */
>> -const char *xs_daemon_rootdir(void);
>> -const char *xs_domain_dev(void);
>> -const char *xs_daemon_tdb(void);
> 
> Someone who was writing bindings might have exposed these without
> knowing what they were, resulting in linkage to these symbols.

This patch is removing everything not being used in the (known) Xen
ecosystem (Xen, qemu, qemu-trad, mini-os).

> 
>>   bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
>>   			 const char *strings);
>>   
>> -/* 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);
> 
> Isn't this function potentially useful ?  It seems funny to have only
> one of the conversion directions.

As stated above: this patch is doing the absolute possible maximum.
I'm absolutely fine to drop some of the removals.

>> +void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
> 
> Is it possible to do sort this out in a more minimal way ?  Eg we
> could change the name to namespace it properly.  (I haven't looked at
> the code in detail and am still rather under-caffeinated so maybe I am
> talking nonsense here.)

No nonsense. This would be the really minimum option (apart from doing
nothing).

I can setup the patch for that and keep the rest for 4.16 (which will
then probably need to bump the so version).


Juergen

Jürgen Groß writes ("Re: [PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface"):
> On 24.03.21 12:02, Ian Jackson wrote:
> > Is it possible to do sort this out in a more minimal way ?  Eg we
> > could change the name to namespace it properly.  (I haven't looked at
> > the code in detail and am still rather under-caffeinated so maybe I am
> > talking nonsense here.)
> 
> No nonsense. This would be the really minimum option (apart from doing
> nothing).
> 
> I can setup the patch for that and keep the rest for 4.16 (which will
> then probably need to bump the so version).

Hmmm.  Maybe it would be less disruptive to punt the whole lot for
xen-next.  That way we don't have a silent withdrawl in one release
followed by a soname bump in the next.

If you're keen to change this for 4.15, please feel free to show me
what the patch looks like.  But I would be inclined to postpone this.

Thanks,
Ian.

On 24.03.21 12:30, Ian Jackson wrote:
> Jürgen Groß writes ("Re: [PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface"):
>> On 24.03.21 12:02, Ian Jackson wrote:
>>> Is it possible to do sort this out in a more minimal way ?  Eg we
>>> could change the name to namespace it properly.  (I haven't looked at
>>> the code in detail and am still rather under-caffeinated so maybe I am
>>> talking nonsense here.)
>>
>> No nonsense. This would be the really minimum option (apart from doing
>> nothing).
>>
>> I can setup the patch for that and keep the rest for 4.16 (which will
>> then probably need to bump the so version).
> 
> Hmmm.  Maybe it would be less disruptive to punt the whole lot for
> xen-next.  That way we don't have a silent withdrawl in one release
> followed by a soname bump in the next.
> 
> If you're keen to change this for 4.15, please feel free to show me
> what the patch looks like.  But I would be inclined to postpone this.

Minimal variant sent. I'm not keen to have that for 4.15, but the patch
was just ready. :-)

Juergen

Jürgen Groß writes ("Re: [PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface"):
> On 24.03.21 12:30, Ian Jackson wrote:
> > If you're keen to change this for 4.15, please feel free to show me
> > what the patch looks like.  But I would be inclined to postpone this.
> 
> Minimal variant sent. I'm not keen to have that for 4.15, but the patch
> was just ready. :-)

Thanks :-).  However, looking at it I think it would be best to
postpone this for xen-next and then do the full fix with soname bump.

Ian.