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

[Juergen Gross <jgross@xxxxxxxx>]

On 06.03.23 09:00, Jan Beulich wrote:
> On 06.03.2023 08:21, Juergen Gross wrote:
> > --- a/tools/include/xen-tools/common-macros.h
> > +++ b/tools/include/xen-tools/common-macros.h
> > @@ -80,4 +80,8 @@
> >       typeof( ((type *)0)->member ) *__mptr = (ptr);        \
> >       (type *)( (char *)__mptr - offsetof(type,member) );})
> >   
> > +#ifndef offsetof
> > +#define offsetof(a, b) __builtin_offsetof(a, b)
> > +#endif
> How's this going to work reliably with parties perhaps also including
> stddef.h, which also is expected to define this macro, and which may
> do so with a different sequence of tokens (even if that ends up
> having the same effect)? There shouldn't be a need to define this
> macro for any "normal" environments, e.g. ...
>
> > --- a/tools/libfsimage/xfs/fsys_xfs.c
> > +++ b/tools/libfsimage/xfs/fsys_xfs.c
> > @@ -19,6 +19,7 @@
> >   
> >   #include <xenfsimage_grub.h>
> >   #include "xfs.h"
> > +#include <xen-tools/common-macros.h>
> >   
> >   #define MAX_LINK_COUNT	8
> >   
> > @@ -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)
> >   {
> ... this library or ...
>
> > --- a/tools/libs/vchan/init.c
> > +++ b/tools/libs/vchan/init.c
> > @@ -69,10 +69,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;
> ... this tool or ...
>
> > --- a/tools/tests/vhpet/emul.h
> > +++ b/tools/tests/vhpet/emul.h
> > @@ -115,8 +115,6 @@ enum
> >       NR_COMMON_SOFTIRQS
> >   };
> >   
> > -#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
> > -
> >   struct domain;
> >   
> >   struct vcpu
> ... this test binary (which is an ordinary host one as well). They all
> should include stddef.h and be done.
Yes, this is probably a better way to handle this.

So I think this patch should be dropped.

I'll send out another small series removing the private offsetof()
definitions and using stddef.h instead. This should include hvmloader,
which can use the tools/firmware/include/stddef.h variant.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature