Re: [PATCH v1] keyhandler: add handler to show Xen command line

On 30.07.2026 08:15, [email protected] wrote:
> From: Denis Mukhin <[email protected]> 

First, you want to update Roger's email address that you use. This is now the
2nd patch that you sent to his old address, and the latest after sending the 1st
you should have noticed.

> Currently there's no way to print Xen command line on the emergency console
> for debugging purposes.
> 
> Add new keyhander 'C' to add (saved) command line printout for debugging
> the system.

If others think spending a precious character on this, I think I won't attempt
to veto this, but I'm definitely not going to ack such. More importantly though
you shouldn't be using a character that's already in use.

Nevertheless a few comments below.

> Also, fix indentation for KEYHANDLER() entries in 'keyhandler' array.

"fix" as in "break"? Patch context alone doesn't make clear at all why you'd
do this. Looking at the full source I think I can see why you think something
may be wrong there, but I don't think it is - the earlier block outside of the
#ifdef-s is different in this regard, imo.

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -232,6 +232,11 @@ void __init cmdline_parse(const char *cmdline)
>  #endif
>  }
>  
> +const char *cmdline_get(void)
> +{
> +    return saved_cmdline;
> +}

To accompany both this use and the pre-existing ones, perhaps it's time to
mark saved_cmdline[] __ro_after_init (in a separate patch preferably)?

> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -28,7 +28,7 @@ static unsigned char keypress_key;
>  static bool alt_key_handling;
>  
>  static keyhandler_fn_t cf_check show_handlers, cf_check dump_hwdom_registers,
> -    cf_check dump_domains, cf_check read_clocks;
> +    cf_check dump_domains, cf_check read_clocks, show_cmdline;
>  static irq_keyhandler_fn_t cf_check do_toggle_alt_key, cf_check dump_registers,
>      cf_check reboot_machine, cf_check run_all_keyhandlers;
>  
> @@ -60,15 +60,17 @@ static struct keyhandler {
>      IRQ_KEYHANDLER('*', run_all_keyhandlers, "print all diagnostics", 0),
>  
>  #ifdef CONFIG_PERF_COUNTERS
> -    KEYHANDLER('p', perfc_printall, "print performance counters", 1),
> -    KEYHANDLER('P', perfc_reset, "reset performance counters", 0),
> +        KEYHANDLER('p', perfc_printall, "print performance counters", 1),
> +        KEYHANDLER('P', perfc_reset, "reset performance counters", 0),
>  #endif
>  
>  #ifdef CONFIG_DEBUG_LOCK_PROFILE
> -    KEYHANDLER('l', spinlock_profile_printall, "print lock profile info", 1),
> -    KEYHANDLER('L', spinlock_profile_reset, "reset lock profile info", 0),
> +        KEYHANDLER('l', spinlock_profile_printall, "print lock profile info", 1),
> +        KEYHANDLER('L', spinlock_profile_reset, "reset lock profile info", 0),
>  #endif
>  
> +        KEYHANDLER('C', show_cmdline, "show saved command line", 0),

Why not insert at the appropriate slot in the earlier block outside of any
#ifdef?

As to the help text - in how far is "saved" relevant there?

> @@ -512,6 +514,12 @@ static void cf_check do_toggle_alt_key(unsigned char key, bool unused)
>             alt_key_handling ? "alternative" : "normal");
>  }
>  
> +static void cf_check show_cmdline(unsigned char key)
> +{
> +    printk("'%c' pressed -> showing saved command line\n", key);
> +    printk("%s\n", cmdline_get());
> +}

If this lived in kernel.c, the new cmdline_get() wouldn't be needed at all.

Jan