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 <jbeul...@suse.com>
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
