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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Wednesday, February 12th, 2025 at 11:25 PM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

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

Sorry, that's because I used xenver_varbuf_op() as a reference.
Fixed in v2.

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

Thanks!
Addressed in v2.

>
> Jan