Re: [PATCH v4] xen/common: add keyhandler to show Xen command line

On 8/11/26 11:09 PM, [email protected] wrote:
> Currently there's no way to print Xen command line on the emergency
> console for debugging purposes when 'xl' is not unavailable.

It looks like 'not' should be dropped. 'unavailable' covers 'not' itself.

> 
> Add new keyhander 'X' to do command line printout.
> 
> Signed-off-by: Denis Mukhin <[email protected]>
> ---
> Changes since v3:
> - print built-in command line too
> - change handler to print command line only
> 
> Changes since v2:
> - account for CONFIG_CMDLINE_OVERRIDE case
> 
> v3: https://lore.kernel.org/xen-devel/[email protected]/
> CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/2751523722
> ---
>   xen/common/kernel.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index d1bef9ac2b2b..8a43e4421002 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -5,6 +5,7 @@
>    */
>   
>   #include <xen/init.h>
> +#include <xen/keyhandler.h>
>   #include <xen/lib.h>
>   #include <xen/errno.h>
>   #include <xen/param.h>
> @@ -505,6 +506,37 @@ static int __init cf_check param_init(void)
>   __initcall(param_init);
>   #endif
>   
> +static void cf_check show_cmdline(unsigned char key)
> +{
> +    const char *builtin_cmdline = CONFIG_CMDLINE;
> +    const char *cmdline;
> +
> +    printk("'%c' pressed -> showing hypervisor command line\n", key);
> +
> +    if ( builtin_cmdline[0] )
> +        cmdline = builtin_cmdline;
> +    else
> +        cmdline = "<NULL>";
> +
> +    printk("Built-in command line: %s\n", cmdline);
> +
> +    if ( IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) )
> +        cmdline = "<NULL> (CONFIG_CMDLINE_OVERRIDE=y)";

Won't be better to use <ignored> instead of <NULL>? Also it reflects the 
comment (But if CONFIG_CMDLINE_OVERRIDE is set to y, @cmdline will be 
ignored.) above cmdline_parse() function better.

> +    else
> +        cmdline = saved_cmdline;

IIUC, saved_cmdline could be empty and I think we should take that into 
account too:

     if ( IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) )
         cmdline = "<ignored> (CONFIG_CMDLINE_OVERRIDE=y)";
     else if ( saved_cmdline[0] )
         cmdline = saved_cmdline;
     else
         cmdline = "<NULL>";

> +
> +    printk("Command line: %s\n", cmdline);
> +}
> +
> +static int __init cf_check misc_init(void)
> +{
> +    register_keyhandler('X', show_cmdline,
> +                        "show hypervisor command line", 0);

Last argument is declared as bool so I think it will be better to use 
false here.

~ Oleksii