xenstore_lib.h is in need to be tidied up a little bit:

- the definition of struct xs_tdb_record_hdr shouldn't be here
- some symbols are not namespaced correctly

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: minimal variant (Ian Jackson)
---
 tools/include/xenstore_lib.h     | 17 ++++-------------
 tools/libs/store/libxenstore.map |  6 +++---
 tools/libs/store/xs.c            | 12 ++++++------
 tools/xenstore/utils.h           | 11 +++++++++++
 tools/xenstore/xenstore_client.c | 12 ++++++------
 5 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
index 4c9b6d1685..f74ad7024b 100644
--- a/tools/include/xenstore_lib.h
+++ b/tools/include/xenstore_lib.h
@@ -43,15 +43,6 @@ 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)
 
@@ -78,18 +69,18 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
 unsigned int xs_count_strings(const char *strings, unsigned int len);
 
 /* Sanitising (quoting) possibly-binary strings. */
-struct expanding_buffer {
+struct xs_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);
+char *xs_expanding_buffer_ensure(struct xs_expanding_buffer *, int min_avail);
 
 /* sanitise_value() may return NULL if malloc fails. */
-char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
+char *xs_sanitise_value(struct xs_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);
+void xs_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..fc1c213f13 100644
--- a/tools/libs/store/libxenstore.map
+++ b/tools/libs/store/libxenstore.map
@@ -42,8 +42,8 @@ VERS_3.0.3 {
 		xs_strings_to_perms;
 		xs_perm_to_string;
 		xs_count_strings;
-		expanding_buffer_ensure;
-		sanitise_value;
-		unsanitise_value;
+		xs_expanding_buffer_ensure;
+		xs_sanitise_value;
+		xs_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..109ea16d1e 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -1358,7 +1358,7 @@ static void *read_thread(void *arg)
 }
 #endif
 
-char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
+char *xs_expanding_buffer_ensure(struct xs_expanding_buffer *ebuf, int min_avail)
 {
 	int want;
 	char *got;
@@ -1379,8 +1379,8 @@ char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
 	return ebuf->buf;
 }
 
-char *sanitise_value(struct expanding_buffer *ebuf,
-		     const char *val, unsigned len)
+char *xs_sanitise_value(struct xs_expanding_buffer *ebuf,
+			const char *val, unsigned len)
 {
 	int used, remain, c;
 	unsigned char *ip;
@@ -1394,7 +1394,7 @@ char *sanitise_value(struct expanding_buffer *ebuf,
 	used = 0;
 	remain = len;
 
-	if (!expanding_buffer_ensure(ebuf, remain + 1))
+	if (!xs_expanding_buffer_ensure(ebuf, remain + 1))
 		return NULL;
 
 	while (remain-- > 0) {
@@ -1405,7 +1405,7 @@ char *sanitise_value(struct expanding_buffer *ebuf,
 			continue;
 		}
 
-		if (!expanding_buffer_ensure(ebuf, used + remain + 5))
+		if (!xs_expanding_buffer_ensure(ebuf, used + remain + 5))
 			/* for "<used>\\nnn<remain>\0" */
 			return 0;
 
@@ -1429,7 +1429,7 @@ char *sanitise_value(struct expanding_buffer *ebuf,
 #undef ADDF
 }
 
-void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
+void xs_unsanitise_value(char *out, unsigned *out_len_r, const char *in)
 {
 	const char *ip;
 	char *op;
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..3d9d399e91 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -40,7 +40,7 @@ enum mode {
 
 static char *output_buf = NULL;
 static int output_pos = 0;
-static struct expanding_buffer ebuf;
+static struct xs_expanding_buffer ebuf;
 
 static int output_size = 0;
 
@@ -203,11 +203,11 @@ static void do_ls(struct xs_handle *h, char *path, int cur_depth, int show_perms
             if (max_width < (linewid + len + TAG_LEN)) {
                 printf(" = \"%.*s\\...\"",
                        (int)(max_width - TAG_LEN - linewid),
-		       sanitise_value(&ebuf, val, len));
+		       xs_sanitise_value(&ebuf, val, len));
             }
             else {
                 linewid += printf(" = \"%s\"",
-				  sanitise_value(&ebuf, val, len));
+				  xs_sanitise_value(&ebuf, val, len));
                 if (show_perms) {
                     putchar(' ');
                     for (linewid++;
@@ -346,7 +346,7 @@ perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh
             if (raw)
                 output_raw(val, len);
             else
-                output("%s\n", sanitise_value(&ebuf, val, len));
+                output("%s\n", xs_sanitise_value(&ebuf, val, len));
             free(val);
             optind++;
             break;
@@ -359,8 +359,8 @@ perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh
                 val = val_spec;
                 len = strlen(val_spec);
             } else {
-                expanding_buffer_ensure(&ebuf, strlen(val_spec)+1);
-                unsanitise_value(ebuf.buf, &len, val_spec);
+                xs_expanding_buffer_ensure(&ebuf, strlen(val_spec)+1);
+                xs_unsanitise_value(ebuf.buf, &len, val_spec);
                 val = ebuf.buf;
             }
             if (!xs_write(xsh, xth, argv[optind], val, len)) {
-- 
2.26.2

Hi Juergen,

On 24/03/2021 11:30, Juergen Gross wrote:
> xenstore_lib.h is in need to be tidied up a little bit:
> 
> - the definition of struct xs_tdb_record_hdr shouldn't be here
> - some symbols are not namespaced correctly
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2: minimal variant (Ian Jackson)
> ---
>   tools/include/xenstore_lib.h     | 17 ++++-------------
>   tools/libs/store/libxenstore.map |  6 +++---
>   tools/libs/store/xs.c            | 12 ++++++------
>   tools/xenstore/utils.h           | 11 +++++++++++
>   tools/xenstore/xenstore_client.c | 12 ++++++------
>   5 files changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
> index 4c9b6d1685..f74ad7024b 100644
> --- a/tools/include/xenstore_lib.h
> +++ b/tools/include/xenstore_lib.h
> @@ -43,15 +43,6 @@ 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)
>   
> @@ -78,18 +69,18 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
>   unsigned int xs_count_strings(const char *strings, unsigned int len);
>   
>   /* Sanitising (quoting) possibly-binary strings. */
> -struct expanding_buffer {
> +struct xs_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);
> +char *xs_expanding_buffer_ensure(struct xs_expanding_buffer *, int min_avail);
>   
>   /* sanitise_value() may return NULL if malloc fails. */
> -char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
> +char *xs_sanitise_value(struct xs_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);
> +void xs_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..fc1c213f13 100644
> --- a/tools/libs/store/libxenstore.map
> +++ b/tools/libs/store/libxenstore.map
> @@ -42,8 +42,8 @@ VERS_3.0.3 {
>   		xs_strings_to_perms;
>   		xs_perm_to_string;
>   		xs_count_strings;
> -		expanding_buffer_ensure;
> -		sanitise_value;
> -		unsanitise_value;
> +		xs_expanding_buffer_ensure;
> +		xs_sanitise_value;
> +		xs_unsanitise_value;

Isn't libxenstore considered stable? If so, shouldn't we bump the 
version to avoid any breakage for existing app?

Cheers,

-- 
Julien Grall

On 24.03.21 15:09, Julien Grall wrote:
> Hi Juergen,
> 
> On 24/03/2021 11:30, Juergen Gross wrote:
>> xenstore_lib.h is in need to be tidied up a little bit:
>>
>> - the definition of struct xs_tdb_record_hdr shouldn't be here
>> - some symbols are not namespaced correctly
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2: minimal variant (Ian Jackson)
>> ---
>>   tools/include/xenstore_lib.h     | 17 ++++-------------
>>   tools/libs/store/libxenstore.map |  6 +++---
>>   tools/libs/store/xs.c            | 12 ++++++------
>>   tools/xenstore/utils.h           | 11 +++++++++++
>>   tools/xenstore/xenstore_client.c | 12 ++++++------
>>   5 files changed, 30 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
>> index 4c9b6d1685..f74ad7024b 100644
>> --- a/tools/include/xenstore_lib.h
>> +++ b/tools/include/xenstore_lib.h
>> @@ -43,15 +43,6 @@ 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)
>> @@ -78,18 +69,18 @@ bool xs_perm_to_string(const struct xs_permissions 
>> *perm,
>>   unsigned int xs_count_strings(const char *strings, unsigned int len);
>>   /* Sanitising (quoting) possibly-binary strings. */
>> -struct expanding_buffer {
>> +struct xs_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);
>> +char *xs_expanding_buffer_ensure(struct xs_expanding_buffer *, int 
>> min_avail);
>>   /* sanitise_value() may return NULL if malloc fails. */
>> -char *sanitise_value(struct expanding_buffer *, const char *val, 
>> unsigned len);
>> +char *xs_sanitise_value(struct xs_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);
>> +void xs_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..fc1c213f13 100644
>> --- a/tools/libs/store/libxenstore.map
>> +++ b/tools/libs/store/libxenstore.map
>> @@ -42,8 +42,8 @@ VERS_3.0.3 {
>>           xs_strings_to_perms;
>>           xs_perm_to_string;
>>           xs_count_strings;
>> -        expanding_buffer_ensure;
>> -        sanitise_value;
>> -        unsanitise_value;
>> +        xs_expanding_buffer_ensure;
>> +        xs_sanitise_value;
>> +        xs_unsanitise_value;
> 
> Isn't libxenstore considered stable? If so, shouldn't we bump the 
> version to avoid any breakage for existing app?

See https://lists.xen.org/archives/html/xen-devel/2021-03/msg01267.html


Juergen

On 24/03/2021 14:33, Jürgen Groß wrote:
> On 24.03.21 15:09, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 24/03/2021 11:30, Juergen Gross wrote:
>>> xenstore_lib.h is in need to be tidied up a little bit:
>>>
>>> - the definition of struct xs_tdb_record_hdr shouldn't be here
>>> - some symbols are not namespaced correctly
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2: minimal variant (Ian Jackson)
>>> ---
>>>   tools/include/xenstore_lib.h     | 17 ++++-------------
>>>   tools/libs/store/libxenstore.map |  6 +++---
>>>   tools/libs/store/xs.c            | 12 ++++++------
>>>   tools/xenstore/utils.h           | 11 +++++++++++
>>>   tools/xenstore/xenstore_client.c | 12 ++++++------
>>>   5 files changed, 30 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
>>> index 4c9b6d1685..f74ad7024b 100644
>>> --- a/tools/include/xenstore_lib.h
>>> +++ b/tools/include/xenstore_lib.h
>>> @@ -43,15 +43,6 @@ 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)
>>> @@ -78,18 +69,18 @@ bool xs_perm_to_string(const struct 
>>> xs_permissions *perm,
>>>   unsigned int xs_count_strings(const char *strings, unsigned int len);
>>>   /* Sanitising (quoting) possibly-binary strings. */
>>> -struct expanding_buffer {
>>> +struct xs_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);
>>> +char *xs_expanding_buffer_ensure(struct xs_expanding_buffer *, int 
>>> min_avail);
>>>   /* sanitise_value() may return NULL if malloc fails. */
>>> -char *sanitise_value(struct expanding_buffer *, const char *val, 
>>> unsigned len);
>>> +char *xs_sanitise_value(struct xs_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);
>>> +void xs_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..fc1c213f13 100644
>>> --- a/tools/libs/store/libxenstore.map
>>> +++ b/tools/libs/store/libxenstore.map
>>> @@ -42,8 +42,8 @@ VERS_3.0.3 {
>>>           xs_strings_to_perms;
>>>           xs_perm_to_string;
>>>           xs_count_strings;
>>> -        expanding_buffer_ensure;
>>> -        sanitise_value;
>>> -        unsanitise_value;
>>> +        xs_expanding_buffer_ensure;
>>> +        xs_sanitise_value;
>>> +        xs_unsanitise_value;
>>
>> Isn't libxenstore considered stable? If so, shouldn't we bump the 
>> version to avoid any breakage for existing app?
> 
> See https://lists.xen.org/archives/html/xen-devel/2021-03/msg01267.html

Thanks! Can the content of the discusison be summarized in the commit 
message? This would avoid other reviewers to wonder why the change is fine.

Cheers,

-- 
Julien Grall

On 24/03/2021 11:30, Juergen Gross wrote:
> xenstore_lib.h is in need to be tidied up a little bit:
>
> - the definition of struct xs_tdb_record_hdr shouldn't be here
> - some symbols are not namespaced correctly
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2: minimal variant (Ian Jackson)
> ---
>  tools/include/xenstore_lib.h     | 17 ++++-------------
>  tools/libs/store/libxenstore.map |  6 +++---
>  tools/libs/store/xs.c            | 12 ++++++------
>  tools/xenstore/utils.h           | 11 +++++++++++
>  tools/xenstore/xenstore_client.c | 12 ++++++------
>  5 files changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
> index 4c9b6d1685..f74ad7024b 100644
> --- a/tools/include/xenstore_lib.h
> +++ b/tools/include/xenstore_lib.h
> @@ -43,15 +43,6 @@ struct xs_permissions
>  	enum xs_perm_type perms;

^ This enum is still a ABI problem, as it has implementation defined
size.  The containing struct is used by xs_perm_to_string().

Substituting for int is probably the easiest option, because no amount
of trickery with the enum values themselves can prevent the compiler
deciding to use a long or larger for the object.

~Andrew

On 24.03.21 12:42, Andrew Cooper wrote:
> On 24/03/2021 11:30, Juergen Gross wrote:
>> xenstore_lib.h is in need to be tidied up a little bit:
>>
>> - the definition of struct xs_tdb_record_hdr shouldn't be here
>> - some symbols are not namespaced correctly
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2: minimal variant (Ian Jackson)
>> ---
>>   tools/include/xenstore_lib.h     | 17 ++++-------------
>>   tools/libs/store/libxenstore.map |  6 +++---
>>   tools/libs/store/xs.c            | 12 ++++++------
>>   tools/xenstore/utils.h           | 11 +++++++++++
>>   tools/xenstore/xenstore_client.c | 12 ++++++------
>>   5 files changed, 30 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/include/xenstore_lib.h b/tools/include/xenstore_lib.h
>> index 4c9b6d1685..f74ad7024b 100644
>> --- a/tools/include/xenstore_lib.h
>> +++ b/tools/include/xenstore_lib.h
>> @@ -43,15 +43,6 @@ struct xs_permissions
>>   	enum xs_perm_type perms;
> 
> ^ This enum is still a ABI problem, as it has implementation defined
> size.  The containing struct is used by xs_perm_to_string().
> 
> Substituting for int is probably the easiest option, because no amount
> of trickery with the enum values themselves can prevent the compiler
> deciding to use a long or larger for the object.

Switching to unsigned int and replacing the enum values with #defines
seems to be the way to go, as the enum values are basically bit mask
values.


Juergen