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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.19 at 22:14, <sstabellini@xxxxxxxxxx> wrote:
> On Tue, 26 Feb 2019, Jan Beulich wrote:
>> >>> On 25.02.19 at 21:50, <sstabellini@xxxxxxxxxx> wrote:
>> > --- 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.

Personally I think the bringing in line with the intended common
pattern should be a prereq patch (or several of them if need be)
in this series.

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

Well, as per the discussion also with Ian on patch 2, there's
independent benefit of having that extra function. So yes, I
continue to think it should be introduced.

Jan



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