[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 5/6] xen/common: use DEFINE_SYMBOL as required
>>> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |