Re: [PATCH v3] xen/console: Skip switching serial input to non existing domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2023 09:19, Michal Orzel wrote:
> @@ -483,15 +485,34 @@ struct domain *console_input_domain(void)
>  
>  static void switch_serial_input(void)
>  {
> -    if ( console_rx == max_init_domid + 1 )
> -    {
> -        console_rx = 0;
> -        printk("*** Serial input to Xen");
> -    }
> -    else
> +    unsigned int next_rx = console_rx + 1;
> +
> +    /*
> +     * Rotate among Xen, dom0 and boot-time created domUs while skipping
> +     * switching serial input to non existing domains.
> +     */
> +    while ( next_rx <= max_console_rx + 1 )
>      {
> -        console_rx++;
> -        printk("*** Serial input to DOM%d", console_rx - 1);
> +        if ( next_rx == max_console_rx + 1 )

Part of the earlier problems stemmed from the comparison being == here.
Could I talk you into using >= instead?

> +        {
> +            console_rx = 0;
> +            printk("*** Serial input to Xen");
> +            break;
> +        }
> +        else

No need for "else" after "break" (or alike). Omitting it will not only
decrease indentation, but also make more visible that the earlier if()
won't "fall through".

> +        {
> +            struct domain *d = rcu_lock_domain_by_id(next_rx - 1);
> +
> +            if ( d )
> +            {
> +                rcu_unlock_domain(d);
> +                console_rx = next_rx;
> +                printk("*** Serial input to DOM%d", console_rx - 1);

While I expect the compiler will transform this to using "next_rx"
here anyway, I think it would be nice if it was written like this
right away.

Since you touch the printk() anyway, please also switch to using the
more applicable %u.

With the adjustments
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

One other transformation for you to consider is to switch to a base
layout like

    unsigned int next_rx = console_rx;
    while ( next_rx++ <= max_console_rx )
    {
        ...
    }

i.e. without a separate increment at the bottom of the loop. Which,
now that I've spelled it out, raises the question of why the outer
loop needs a condition in the first place (because as written above
it clearly is always true). So perhaps better (and more directly
showing what's going on)

    unsigned int next_rx = console_rx;
    for ( ; ; )
    {
        if ( next_rx++ >= max_console_rx )
        ...
    }

Jan