[PATCH v4 0/5] tools: use xen-tools/libs.h for common definitions

[PATCH v4 0/5] tools: use xen-tools/libs.h for common definitions

[Juergen Gross]

There are some macros defined multiple times in tools. Use only
a single header file for defining those macros and drop the copies,
or use stddef.h for offsetof().

V2:
- add patch 1 (Andrew Cooper)

V3:
- address comments

V4:
- patch 1 of V3 already applied
- new patches 3-5

Juergen Gross (5):
  tools: add container_of() macro to xen-tools/common-macros.h
  tools: get rid of additional min() and max() definitions
  tools/hvmloader: remove private offsetof() definition
  tools/libfsimage: remove private offsetof() definition
  tools/libs/vchan: remove private offsetof() definition

 tools/firmware/hvmloader/util.h         | 11 ++---------
 tools/include/xen-tools/common-macros.h |  4 ++++
 tools/libfsimage/xfs/fsys_xfs.c         |  4 +---
 tools/libs/vchan/init.c                 |  8 ++------
 tools/tests/vpci/Makefile               |  2 +-
 tools/tests/vpci/emul.h                 | 22 +---------------------
 tools/tests/x86_emulator/x86-emulate.h  |  5 -----
 tools/xenstore/list.h                   |  6 ++----
 8 files changed, 13 insertions(+), 49 deletions(-)

-- 
2.35.3

[PATCH v4 1/5] tools: add container_of() macro to xen-tools/common-macros.h

[Juergen Gross]

Instead of having 4 identical copies of the definition of a
container_of() macro in different tools header files, add that macro
to xen-tools/common-macros.h and use that instead.

Delete the other copies of that macro.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
There is a similar macro CONTAINER_OF() defined in
tools/include/xentoolcore_internal.h, which allows to not only use a
type for the 2nd parameter, but a variable, too.
I'd like to get rid of that macro as well, but there are lots of use
cases especially in libxl. Any thoughts regarding that macro?
I could either:
- don't touch it at all
- enhance container_of() like CONTAINER_OF() and replace all use cases
  of CONTAINER_OF() with container_of()
- replace the few CONTAINER_OF() users outside libxl with container_of()
  and define CONTAINER_OF() in e.g. libxl_internal.h
- replace all CONTAINER_OF() use cases with container_of(), including
  the change from (.., var, ..) to (.., type, ...).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/include/xen-tools/common-macros.h | 4 ++++
 tools/tests/vpci/emul.h                 | 6 +-----
 tools/tests/x86_emulator/x86-emulate.h  | 5 -----
 tools/xenstore/list.h                   | 6 ++----
 4 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index a372b9ecf2..b046ab48bf 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -76,4 +76,8 @@
 #define __must_check __attribute__((__warn_unused_result__))
 #endif
 
+#define container_of(ptr, type, member) ({                \
+    typeof( ((type *)0)->member ) *__mptr = (ptr);        \
+    (type *)( (char *)__mptr - offsetof(type,member) );})
+
 #endif	/* __XEN_TOOLS_COMMON_MACROS__ */
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index f03e3a56d1..7169a2ea02 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -27,11 +27,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 
-#define container_of(ptr, type, member) ({                      \
-        typeof(((type *)0)->member) *mptr = (ptr);              \
-                                                                \
-        (type *)((char *)mptr - offsetof(type, member));        \
-})
+#include <xen-tools/common-macros.h>
 
 #define smp_wmb()
 #define prefetch(x) __builtin_prefetch(x)
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index 46d4e43cea..1af986f78d 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -56,11 +56,6 @@
 
 #define cf_check /* No Control Flow Integriy checking */
 
-#define container_of(ptr, type, member) ({             \
-    typeof(((type *)0)->member) *mptr__ = (ptr);       \
-    (type *)((char *)mptr__ - offsetof(type, member)); \
-})
-
 #define AC_(n,t) (n##t)
 #define _AC(n,t) AC_(n,t)
 
diff --git a/tools/xenstore/list.h b/tools/xenstore/list.h
index b17d13e0ec..a464a38b61 100644
--- a/tools/xenstore/list.h
+++ b/tools/xenstore/list.h
@@ -3,6 +3,8 @@
 /* Taken from Linux kernel code, but de-kernelized for userspace. */
 #include <stddef.h>
 
+#include <xen-tools/common-macros.h>
+
 #undef LIST_HEAD_INIT
 #undef LIST_HEAD
 #undef INIT_LIST_HEAD
@@ -15,10 +17,6 @@
 #define LIST_POISON1  ((void *) 0x00100100)
 #define LIST_POISON2  ((void *) 0x00200200)
 
-#define container_of(ptr, type, member) ({			\
-        typeof( ((type *)0)->member ) *__mptr = (ptr);	\
-        (type *)( (char *)__mptr - offsetof(type,member) );})
-
 /*
  * Simple doubly linked list implementation.
  *
-- 
2.35.3

Re: [PATCH v4 1/5] tools: add container_of() macro to xen-tools/common-macros.h

[Jan Beulich]

On 22.03.2023 13:08, Juergen Gross wrote:
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -76,4 +76,8 @@
>  #define __must_check __attribute__((__warn_unused_result__))
>  #endif
>  
> +#define container_of(ptr, type, member) ({                \
> +    typeof( ((type *)0)->member ) *__mptr = (ptr);        \
> +    (type *)( (char *)__mptr - offsetof(type,member) );})

Can the variant used here please be closer to ...

> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -56,11 +56,6 @@
>  
>  #define cf_check /* No Control Flow Integriy checking */
>  
> -#define container_of(ptr, type, member) ({             \
> -    typeof(((type *)0)->member) *mptr__ = (ptr);       \
> -    (type *)((char *)mptr__ - offsetof(type, member)); \
> -})

... this rather than ...

> --- a/tools/xenstore/list.h
> +++ b/tools/xenstore/list.h
> @@ -3,6 +3,8 @@
>  /* Taken from Linux kernel code, but de-kernelized for userspace. */
>  #include <stddef.h>
>  
> +#include <xen-tools/common-macros.h>
> +
>  #undef LIST_HEAD_INIT
>  #undef LIST_HEAD
>  #undef INIT_LIST_HEAD
> @@ -15,10 +17,6 @@
>  #define LIST_POISON1  ((void *) 0x00100100)
>  #define LIST_POISON2  ((void *) 0x00200200)
>  
> -#define container_of(ptr, type, member) ({			\
> -        typeof( ((type *)0)->member ) *__mptr = (ptr);	\
> -        (type *)( (char *)__mptr - offsetof(type,member) );})

... this, both formatting-wise (excess blanks) and local-variable-
naming-wise (trailing underscores instead of leading ones)? (If I was
the one to commit this, I'd be happy to make the adjustment at that
time.) Then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v4 1/5] tools: add container_of() macro to xen-tools/common-macros.h

[Juergen Gross]

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

On 22.03.23 13:34, Jan Beulich wrote:
> On 22.03.2023 13:08, Juergen Gross wrote:
>> --- a/tools/include/xen-tools/common-macros.h
>> +++ b/tools/include/xen-tools/common-macros.h
>> @@ -76,4 +76,8 @@
>>   #define __must_check __attribute__((__warn_unused_result__))
>>   #endif
>>   
>> +#define container_of(ptr, type, member) ({                \
>> +    typeof( ((type *)0)->member ) *__mptr = (ptr);        \
>> +    (type *)( (char *)__mptr - offsetof(type,member) );})
> 
> Can the variant used here please be closer to ...
> 
>> --- a/tools/tests/x86_emulator/x86-emulate.h
>> +++ b/tools/tests/x86_emulator/x86-emulate.h
>> @@ -56,11 +56,6 @@
>>   
>>   #define cf_check /* No Control Flow Integriy checking */
>>   
>> -#define container_of(ptr, type, member) ({             \
>> -    typeof(((type *)0)->member) *mptr__ = (ptr);       \
>> -    (type *)((char *)mptr__ - offsetof(type, member)); \
>> -})
> 
> ... this rather than ...
> 
>> --- a/tools/xenstore/list.h
>> +++ b/tools/xenstore/list.h
>> @@ -3,6 +3,8 @@
>>   /* Taken from Linux kernel code, but de-kernelized for userspace. */
>>   #include <stddef.h>
>>   
>> +#include <xen-tools/common-macros.h>
>> +
>>   #undef LIST_HEAD_INIT
>>   #undef LIST_HEAD
>>   #undef INIT_LIST_HEAD
>> @@ -15,10 +17,6 @@
>>   #define LIST_POISON1  ((void *) 0x00100100)
>>   #define LIST_POISON2  ((void *) 0x00200200)
>>   
>> -#define container_of(ptr, type, member) ({			\
>> -        typeof( ((type *)0)->member ) *__mptr = (ptr);	\
>> -        (type *)( (char *)__mptr - offsetof(type,member) );})
> 
> ... this, both formatting-wise (excess blanks) and local-variable-
> naming-wise (trailing underscores instead of leading ones)? (If I was
> the one to commit this, I'd be happy to make the adjustment at that
> time.) Then

Yes, absolutely fine with me.

> Acked-by: Jan Beulich <jbeulich@suse.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 --]

[PATCH v4 2/5] tools: get rid of additional min() and max() definitions

[Juergen Gross]

Defining min(), min_t(), max() and max_t() at other places than
xen-tools/common-macros.h isn't needed, as the definitions in said
header can be used instead.

Same applies to BUILD_BUG_ON() in hvmloader.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/firmware/hvmloader/util.h |  8 ++------
 tools/libs/vchan/init.c         |  3 +--
 tools/tests/vpci/Makefile       |  2 +-
 tools/tests/vpci/emul.h         | 16 ----------------
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 8d95eab28a..e04990ee97 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -9,6 +9,8 @@
 #include <xen/hvm/hvm_info_table.h>
 #include "e820.h"
 
+#include <xen-tools/common-macros.h>
+
 /* Request un-prefixed values from errno.h. */
 #define XEN_ERRNO(name, value) name = value,
 enum {
@@ -41,12 +43,6 @@ void __assert_failed(const char *assertion, const char *file, int line)
 void __bug(const char *file, int line) __attribute__((noreturn));
 #define BUG() __bug(__FILE__, __LINE__)
 #define BUG_ON(p) do { if (p) BUG(); } while (0)
-#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * !!(p)]))
-
-#define min_t(type,x,y) \
-        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
-#define max_t(type,x,y) \
-        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
 
 #define MB(mb) (mb##ULL << 20)
 #define GB(gb) (gb##ULL << 30)
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 9195bd3b98..021e1f29e1 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -45,6 +45,7 @@
 #include <xen/sys/gntalloc.h>
 #include <xen/sys/gntdev.h>
 #include <libxenvchan.h>
+#include <xen-tools/common-macros.h>
 
 #include "vchan.h"
 
@@ -72,8 +73,6 @@
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
 #endif
 
-#define max(a,b) ((a > b) ? a : b)
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
 	int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index 5075bc2be2..62f21f341a 100644
--- a/tools/tests/vpci/Makefile
+++ b/tools/tests/vpci/Makefile
@@ -11,7 +11,7 @@ run: $(TARGET)
 	./$(TARGET)
 
 $(TARGET): vpci.c vpci.h list.h main.c emul.h
-	$(HOSTCC) -g -o $@ vpci.c main.c
+	$(HOSTCC) $(CFLAGS_xeninclude) -g -o $@ vpci.c main.c
 
 .PHONY: clean
 clean:
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 7169a2ea02..8c5bcadd5f 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -106,22 +106,6 @@ typedef union {
 #define BUG() assert(0)
 #define ASSERT_UNREACHABLE() assert(0)
 
-#define min(x, y) ({                    \
-        const typeof(x) tx = (x);       \
-        const typeof(y) ty = (y);       \
-                                        \
-        (void) (&tx == &ty);            \
-        tx < ty ? tx : ty;              \
-})
-
-#define max(x, y) ({                    \
-        const typeof(x) tx = (x);       \
-        const typeof(y) ty = (y);       \
-                                        \
-        (void) (&tx == &ty);            \
-        tx > ty ? tx : ty;              \
-})
-
 #endif
 
 /*
-- 
2.35.3

Re: [PATCH v4 2/5] tools: get rid of additional min() and max() definitions

[Jan Beulich]

On 22.03.2023 13:08, Juergen Gross wrote:
> --- a/tools/tests/vpci/emul.h
> +++ b/tools/tests/vpci/emul.h
> @@ -106,22 +106,6 @@ typedef union {
>  #define BUG() assert(0)
>  #define ASSERT_UNREACHABLE() assert(0)
>  
> -#define min(x, y) ({                    \
> -        const typeof(x) tx = (x);       \
> -        const typeof(y) ty = (y);       \
> -                                        \
> -        (void) (&tx == &ty);            \
> -        tx < ty ? tx : ty;              \
> -})
> -
> -#define max(x, y) ({                    \
> -        const typeof(x) tx = (x);       \
> -        const typeof(y) ty = (y);       \
> -                                        \
> -        (void) (&tx == &ty);            \
> -        tx > ty ? tx : ty;              \
> -})

The new include is added to this file by the first patch. How do things
build warning-free before these macros (functionally compatible but
different in the specific tokens used) are removed, i.e. before this
patch is (also) applied?

Jan

Re: [PATCH v4 2/5] tools: get rid of additional min() and max() definitions

[Juergen Gross]

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

On 22.03.23 13:38, Jan Beulich wrote:
> On 22.03.2023 13:08, Juergen Gross wrote:
>> --- a/tools/tests/vpci/emul.h
>> +++ b/tools/tests/vpci/emul.h
>> @@ -106,22 +106,6 @@ typedef union {
>>   #define BUG() assert(0)
>>   #define ASSERT_UNREACHABLE() assert(0)
>>   
>> -#define min(x, y) ({                    \
>> -        const typeof(x) tx = (x);       \
>> -        const typeof(y) ty = (y);       \
>> -                                        \
>> -        (void) (&tx == &ty);            \
>> -        tx < ty ? tx : ty;              \
>> -})
>> -
>> -#define max(x, y) ({                    \
>> -        const typeof(x) tx = (x);       \
>> -        const typeof(y) ty = (y);       \
>> -                                        \
>> -        (void) (&tx == &ty);            \
>> -        tx > ty ? tx : ty;              \
>> -})
> 
> The new include is added to this file by the first patch. How do things
> build warning-free before these macros (functionally compatible but
> different in the specific tokens used) are removed, i.e. before this
> patch is (also) applied?

Oh, I seem to have failed to make a test build of the tools/tests directory
after applying only the first patch.

It would be probably be the best to switch patches 1 and 2 while keeping the
"#include <xen-tools/common-macros.h>" in tools/tests/vpci/emul.h in the first
patch.


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

[PATCH v4 3/5] tools/hvmloader: remove private offsetof() definition

[Juergen Gross]

util.h contains a definition of offsetof(), which isn't needed.

Remove it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
---
 tools/firmware/hvmloader/util.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index e04990ee97..7249773eeb 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -30,9 +30,6 @@ enum {
 #define SEL_DATA32          0x0020
 #define SEL_CODE64          0x0028
 
-#undef offsetof
-#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
-
 #undef NULL
 #define NULL ((void*)0)
 
-- 
2.35.3

Re: [PATCH v4 3/5] tools/hvmloader: remove private offsetof() definition

[Jan Beulich]

On 22.03.2023 13:08, Juergen Gross wrote:
> util.h contains a definition of offsetof(), which isn't needed.

While true, this is also ambiguous in the context of this series: It isn't
"not needed" because common-macros.h has another definition, but it is
actually unused in hvmloader code. So perhaps s/needed/used/?

> Remove it.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v4 3/5] tools/hvmloader: remove private offsetof() definition

[Juergen Gross]

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

On 22.03.23 13:42, Jan Beulich wrote:
> On 22.03.2023 13:08, Juergen Gross wrote:
>> util.h contains a definition of offsetof(), which isn't needed.
> 
> While true, this is also ambiguous in the context of this series: It isn't
> "not needed" because common-macros.h has another definition, but it is
> actually unused in hvmloader code. So perhaps s/needed/used/?

Fine with me.

> 
>> Remove it.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.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 --]

[PATCH v4 4/5] tools/libfsimage: remove private offsetof() definition

[Juergen Gross]

xfs/fsys_xfs.c is defining offsetof privately. Remove that definition
and just use stddef.h instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
---
 tools/libfsimage/xfs/fsys_xfs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/libfsimage/xfs/fsys_xfs.c b/tools/libfsimage/xfs/fsys_xfs.c
index d735a88e55..b8b4ca928c 100644
--- a/tools/libfsimage/xfs/fsys_xfs.c
+++ b/tools/libfsimage/xfs/fsys_xfs.c
@@ -17,6 +17,7 @@
  *  along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <stddef.h>
 #include <xenfsimage_grub.h>
 #include "xfs.h"
 
@@ -182,9 +183,6 @@ fsb2daddr (xfs_fsblock_t fsbno)
 			 (xfs_agblock_t)(fsbno & mask32lo(xfs.agblklog)));
 }
 
-#undef offsetof
-#define offsetof(t,m)	((size_t)&(((t *)0)->m))
-
 static inline int
 btroot_maxrecs (fsi_file_t *ffi)
 {
-- 
2.35.3

[PATCH v4 5/5] tools/libs/vchan: remove private offsetof() definition

[Juergen Gross]

vchan/init.c is defining offsetof privately. Remove that definition
and just use stddef.h instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
---
 tools/libs/vchan/init.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index 021e1f29e1..a081dd4c9a 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -32,6 +32,7 @@
 #include <sys/mman.h>
 #include <sys/ioctl.h>
 #include <sys/user.h>
+#include <stddef.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -69,10 +70,6 @@
 #define MAX_RING_SHIFT 20
 #define MAX_RING_SIZE (1 << MAX_RING_SHIFT)
 
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
-#endif
-
 static int init_gnt_srv(struct libxenvchan *ctrl, int domain)
 {
 	int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
-- 
2.35.3