Re: [PATCH v2 1/5] symbols: add minimal self-test

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Apr 02, 2025 at 03:57:57PM +0200, Jan Beulich wrote:
> ... before making changes to the involved logic.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While Andrew validly suggests cf_check isn't a requirement for selecting
> which function(s) to use (with the non-upstream gcc patch that we're
> using in CI), that's only because of how the non-upstream patch works.
> Going function-pointer -> unsigned long -> function-pointer without it
> being diagnosed that the cf_check is missing is a shortcoming there, and
> might conceivably be fixed at some point. (Imo any address-taking on a
> function should require it to be cf_check.) Hence I'd like to stick to
> using cf_check functions only for passing to test_lookup().
> 
> With this FAST_SYMBOL_LOOKUP may make sense to permit enabling even
> when LIVEPATCH=n. Thoughts? (In this case "symbols: centralize and re-
> arrange $(all_symbols) calculation" would want pulling ahead.)
> 
> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -260,6 +260,41 @@ unsigned long symbols_lookup_by_name(con
>      return 0;
>  }
>  
> +#ifdef CONFIG_SELF_TESTS
> +
> +static void __init test_lookup(unsigned long addr, const char *expected)
> +{
> +    char buf[KSYM_NAME_LEN + 1];
> +    const char *name, *symname;
> +    unsigned long size, offs;
> +
> +    name = symbols_lookup(addr, &size, &offs, buf);
> +    if ( !name )
> +        panic("%s: address not found\n", expected);
> +    if ( offs )
> +        panic("%s: non-zero offset (%#lx) unexpected\n", expected, offs);

If there's a non-zero offset returned, could you also print the
returned name? (so use %s+%#lx) there's a change the returned name
doesn't match what we expect if there's a non-zero offset.

The rest LGTM:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.