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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

> --- 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.

> @@ -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.

Jan



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