Re: [PATCH] xen/console: add keyhandler to print Xen version

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2025 00:58, dmkhn@xxxxxxxxx wrote:
> Add Xen version printout via keyhandler mechanism.
> 
> That is useful for debugging systems that have been left intact for a long
> time.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

First, +1 to what Andrew said.

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -974,6 +974,28 @@ void guest_printk(const struct domain *d, const char 
> *fmt, ...)
>      va_end(args);
>  }
>  
> +static void xen_print_version(void)
> +{
> +    const char *str = NULL;
> +    unsigned int str_len;
> +
> +    printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
> +           xen_major_version(), xen_minor_version(), xen_extra_version(),
> +           xen_compile_by(), xen_compile_domain(), xen_compiler(),
> +           xen_build_info(), xen_compile_date());
> +
> +    printk("Latest ChangeSet: %s\n", xen_changeset());
> +
> +    if ( !xen_build_id((const void **)&str, &str_len) )

Why the cast? What wrong with "str" being const void *? The only thing
is that it's then not really a good name for the variable, but that it
isn't anyway. It's not really a string you get back.

> @@ -1024,15 +1046,12 @@ void __init console_init_preirq(void)
>      nrspin_lock(&console_lock);
>      __putstr(xen_banner());
>      nrspin_unlock(&console_lock);
> -    printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
> -           xen_major_version(), xen_minor_version(), xen_extra_version(),
> -           xen_compile_by(), xen_compile_domain(), xen_compiler(),
> -           xen_build_info(), xen_compile_date());
> -    printk("Latest ChangeSet: %s\n", xen_changeset());
>  
>      /* Locate and print the buildid, if applicable. */
>      xen_build_init();
>  
> +    xen_print_version();

It may seem minor, but I'm not happy about the reordering. What's logged
would better really be directly after the banner. Any present or future
output from xen_build_init() should come afterwards. Simply add
xen_print_buildid() along with xen_print_version()? And then have it in
version.c, where you can use build_id_{p,len} directly, eliminating the
point above regarding casts and naming.

Jan