Re: [Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.01.19 at 00:42, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -147,14 +147,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 ( SYMBOL(__note_gnu_build_id_end) <= &n[0] )
>          return -ENODATA;
>  
>      /* Check for full Note header. */
> -    if ( &n[1] >= __note_gnu_build_id_end )
> +    if ( &n[1] >= SYMBOL(__note_gnu_build_id_end) )
>          return -ENODATA;
>  
> -    sz = (void *)__note_gnu_build_id_end - (void *)n;
> +    sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;

Now this is an instance where I wouldn't mind if you switched the
casts to (unsigned long).

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -103,13 +103,13 @@ void __init setup_virtual_regions(const struct 
> exception_table_entry *start,
>  {
>      size_t sz;
>      unsigned int i;
> -    static const struct bug_frame *const __initconstrel bug_frames[] = {
> -        __start_bug_frames,
> -        __stop_bug_frames_0,
> -        __stop_bug_frames_1,
> -        __stop_bug_frames_2,
> +    const struct bug_frame *bug_frames[] = {

Please don't loose the second const.

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,27 +66,27 @@
>  })
>  
>  extern char _start[], _end[], start[];
> -#define is_kernel(p) ({                         \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _start) && (__p < _end);            \
> +#define is_kernel(p) ({                                             \
> +    char *p__ = (char *)(unsigned long)(p);                         \
> +    (p__ >= SYMBOL(_start)) && (p__ < SYMBOL(_end));                \
>  })
>  
>  extern char _stext[], _etext[];
> -#define is_kernel_text(p) ({                    \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _stext) && (__p < _etext);          \
> +#define is_kernel_text(p) ({                                        \
> +    char *p__ = (char *)(unsigned long)(p);                         \
> +    (p__ >= SYMBOL(_stext)) && (p__ < SYMBOL(_etext));              \
>  })
>  
>  extern const char _srodata[], _erodata[];
> -#define is_kernel_rodata(p) ({                  \
> -    const char *__p = (const char *)(unsigned long)(p);     \
> -    (__p >= _srodata) && (__p < _erodata);      \
> +#define is_kernel_rodata(p) ({                                      \
> +    const char *p__ = (const char *)(unsigned long)(p);             \

Just like here, in all other of the sibling macros you could easily
have switched p__ to be const char * as well.

With at least the bug_frames[] remark taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



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