Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Juergen Gross <jgross@xxxxxxxx>]

On 27.02.23 15:06, Andrew Cooper wrote:
> On 27/02/2023 12:43 pm, Jan Beulich wrote:
> > On 27.02.2023 13:34, Juergen Gross wrote:
> > > On 27.02.23 13:31, Jan Beulich wrote:
> > > > On 27.02.2023 13:09, Juergen Gross wrote:
> > > > > --- 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)
> > > > >    
> > > > > diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
> > > > > index c7f974608a..926ae12fa5 100644
> > > > > --- a/tools/firmware/include/stddef.h
> > > > > +++ b/tools/firmware/include/stddef.h
> > > > > @@ -1,10 +1,10 @@
> > > > >    #ifndef _STDDEF_H_
> > > > >    #define _STDDEF_H_
> > > > >    
> > > > > +#include <xen-tools/libs.h>
> > > > I'm not convinced firmware/ files can validly include this header.
> > > This file only contains macros which don't call any external functions.
> > >
> > > Would you be fine with me adding a related comment to it?
> > If it was to be usable like this, that would be a prereq, but personally
> > I don't view this as sufficient. The environment code runs in that lives
> > under firmware/ is simply different (hvmloader, for example, is 32-bit
> > no matter that the toolstack would normally be 64-bit), so to me it
> > feels like setting up a trap for ourselves. If Andrew or Roger are fully
> > convinced this is a good move, I won't be standing in the way ...
> We still support 32bit builds of the toolstack on multiple
> architectures, so I don't see bitness as an argument against.
>
> I am slightly uneasy adding a header like this, but it really is only
> common macros and I don't see how it could possibly change from that
> given the existing uses.  OTOH, removing things like the problematic
> definition of offsetof() is an improvement.
>
> I'd probably tentatively ack this patch, seeing as it is a minor
> improvlement, but I think there's a middle option too.  Rename libs.h to
> macros.h or common-macros.h?  IMO that would be something that's far
> more obviously safe to include into firmware/, and something rather more
> descriptive at all of its include sites.
>
> Thoughts?
I'm fine with that.

My preference would be "common-macros.h".


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature