| Message ID | 20230306072140.28402-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: use xen-tools/libs.h for common definitions | expand |
On Mon, Mar 06, 2023 at 08:21:38AM +0100, Juergen Gross wrote: > Instead of having 4 identical copies of the definition of a 3 now ;-), as tests/vhpet has been removed. > 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, ...). I would like to keep the functionality where we can use a type or a var, as this is more convenient. Even Linux developer wants this extra functionality as I've seen a couple of use of container_of() with "typeof(*var)" in the 2nd parameter (in linux source code). I don't know if having our macro "container_of()" been different than the one from Linux or QEMU is going to be an issue, if that's not likely to be an issue we could add the functionality, but if it's likely to be an issue we could instead replace "container_of()" by "CONTAINER_OF()". The issue that I could see with adding "*var" option to container_of() is if someone copies code from Xen into other projects, and not realizing that container_of() is different. While if we spell it "CONTAINER_OF()" instead, it would be less of an issue as the macro would just be missing. (But maybe the first case just mean the compiler will complain) So I'm in favor of having only one macro, with the functionality of the existing "CONTAINER_OF()" macro, and I guess spell it "CONTAINER_OF()" instead of "container_of()". Unless you think the lower case spelling isn't likely to be an issue. > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/include/xen-tools/common-macros.h | 4 ++++ > tools/tests/vhpet/emul.h | 3 --- > tools/tests/vpci/emul.h | 6 +----- > tools/tests/x86_emulator/x86-emulate.h | 5 ----- > tools/xenstore/list.h | 6 ++---- > 5 files changed, 7 insertions(+), 17 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); \ I think identifier starting with two '_' are supposed to be reserved. Would you be ok to have just one? (So "_mptr") > + (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> This doesn't build, so some change are needed in the Makefile. I wondered why the gitlab ci was green while this failed to build on my machine, and it turns out that the "default" target been used is often "install", which does nothing for vpci. But I guess this should be fixed by https://lore.kernel.org/r/20230313121226.86557-1-roger.pau@citrix.com Cheers,
On 23.03.2023 11:44, Anthony PERARD wrote: > On Mon, Mar 06, 2023 at 08:21:38AM +0100, 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); \ > > I think identifier starting with two '_' are supposed to be reserved. > Would you be ok to have just one? (So "_mptr") Except that single-underscore prefixed names are also kind of reserved (for file-scope symbols). Hence why I'm generally suggesting / asking for trailing underscores to be used. Jan
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/vhpet/emul.h b/tools/tests/vhpet/emul.h index dfeb10f74e..610641ab0c 100644 --- a/tools/tests/vhpet/emul.h +++ b/tools/tests/vhpet/emul.h @@ -125,9 +125,6 @@ enum #define max_t(type, x, y) \ ({ type __x = (x); type __y = (y); __x > __y ? __x : __y; }) #define offsetof(t, m) ((unsigned long )&((t *)0)->m) -#define container_of(ptr, type, member) ({ \ - typeof( ((type *)0)->member ) *__mptr = (ptr); \ - (type *)( (char *)__mptr - offsetof(type,member) ); }) struct domain; 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. *