Re: [PATCH v3 2/4] tools: add container_of() macro to xen-tools/common-macros.h

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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@xxxxxxxx>
> ---
>  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@xxxxxxxxxx


Cheers,

-- 
Anthony PERARD