Re: [Xen-devel] [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 26 Feb 2019, Jan Beulich wrote:
> >>> On 25.02.19 at 21:50, <sstabellini@xxxxxxxxxx> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -306,20 +306,28 @@ void add_taint(unsigned int flag)
> >      tainted |= flag;
> >  }
> >  
> > +/*
> > + * We cannot use DEFINE_SYMBOL because we have three variables instead
> > + * of two
> > + */
> >  extern const initcall_t __initcall_start[], __presmp_initcall_end[],
> >      __initcall_end[];
> 
> But I did mention this specific case on the phone call we had: You
> simply want to split this into two proper ranges,
> (__initcall_start,__initcall_end) and
> (__presmp_initcall_start,__presmp_initcall_end). Otherwise ...
> 
> >  void __init do_presmp_initcalls(void)
> >  {
> >      const initcall_t *call;
> > -    for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> > +    for ( call = __initcall_start;
> > +          (uintptr_t)call < (uintptr_t)__presmp_initcall_end;
> > +          call++ )
> >          (*call)();
> 
> ... you could as well use the open coded casting approach everywhere
> (which I then would again grumble about).

I'll follow your suggestion of splitting the ranges.


> > --- a/xen/common/lib.c
> > +++ b/xen/common/lib.c
> > @@ -492,12 +492,15 @@ unsigned long long parse_size_and_unit(const char *s, 
> > const char **ps)
> >  }
> >  
> >  typedef void (*ctor_func_t)(void);
> > -extern const ctor_func_t __ctors_start[], __ctors_end[];
> > +DEFINE_SYMBOL(ctor_func_t, ctor_func, __ctors_start, __ctors_end);
> 
> At the example of this, there's too much redundancy here for my
> taste. At least the _start and _end suffixes should be made
> consistent across the code base (maybe except for the pseudo-
> standard symbols like _etext and _edata). I'd also prefer if what
> is now the first parameter would simply become <second>##_t.
> There's nothing wrong with adding a few typedefs for this to work
> in the common case.

I understand your point but I would prefer to avoid changing the
existing types or names. Instead, I could add a wrapper around
DEFINE_SYMBOL or DECLARE_BOUNDS as you suggested, see my other reply
https://marc.info/?l=xen-devel&m=155120735032147.

However, this example doesn't actually follow the regular pattern
unfortunately (__ctors_start != ctors_func_start). I would like to avoid
making all names/types follow the regular pattern as part of this
series. I could do as a clean-up afterwards.


> > @@ -147,14 +148,14 @@ static int __init xen_build_init(void)
> >      int rc;
> >  
> >      /* --build-id invoked with wrong parameters. */
> > -    if ( __note_gnu_build_id_end <= &n[0] )
> > +    if ( !elf_note_lt(&n[0], __note_gnu_build_id_end) )
> >          return -ENODATA;
> >  
> >      /* Check for full Note header. */
> > -    if ( &n[1] >= __note_gnu_build_id_end )
> > +    if ( !elf_note_lt(&n[1], __note_gnu_build_id_end) )
> >          return -ENODATA;
> >  
> > -    sz = (void *)__note_gnu_build_id_end - (void *)n;
> > +    sz = (uintptr_t)__note_gnu_build_id_end - (uintptr_t)n;
> 
> This is another example of undue open coding. As also said on
> the call we've had, it may be helpful to have a second diff
> function for this, producing the byte difference instead of the
> element one. In fact I did suggest to make the latter use the
> former, such that the casting was truly limited to as few places
> as possible.

I considered adding a second function, but this is the only instance we
have today, so I decided to skip it. I am OK with adding the separate
function though, let me know.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel