Re: [PATCH] Fixed '-serial none' usage breaks following '-serial ...' usage

[Peter Maydell]

(I've cc'd a few people who might have opinions on possible
command-line compatibility breakage.)

On Wed, 10 Jan 2024 at 14:38, Bohdan Kostiv <bogdan.kostiv@gmail.com> wrote:
>
> Hello,
>
> I have faced an issue in using serial ports when I need to skip a couple of 
> ports in the CLI.
>
> For example the ARM machine netduinoplus2 supports up to 7 UARTS.
> Following case works (the first UART is used to send data in the firmware):
> qemu-system-arm -machine netduinoplus2 -nographic -serial mon:stdio -kernel 
> path-to-fw/firmware.elf
> But this one doesn't  (the third UART is used to send data in the firmware):
> qemu-system-arm -machine netduinoplus2 -nographic -serial none -serial none 
> -serial mon:stdio -kernel path-to-fw/firmware.elf

Putting the patch inline for more convenient discussion:

> Subject: [PATCH] Fixed '-serial none' usage breaks following '-serial ...' 
> usage
>
> Signed-off-by: Bohdan Kostiv <bohdan.kostiv@tii.ae>
> ---
>  system/vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index 2bcd9efb9a..b8744475cd 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1442,8 +1442,11 @@ static int serial_parse(const char *devname)
>      int index = num_serial_hds;
>      char label[32];
>
> -    if (strcmp(devname, "none") == 0)
> +    if (strcmp(devname, "none") == 0) {
> +        num_serial_hds++;
>          return 0;
> +    }
> +
>      snprintf(label, sizeof(label), "serial%d", index);
>      serial_hds = g_renew(Chardev *, serial_hds, index + 1);
>
> --
> 2.39.3 (Apple Git-145)

I agree that it's the right thing to do -- '-serial none
-serial foo' ought to set serial_hds(0) as 'none' and
serial_hds(1) as 'foo'.

My only concern here is that this is a very very
longstanding bug -- as far as I can see it was
introduced in commit 998bbd74b9d81 in 2009. So I am
a little worried that maybe some existing command lines
accidentally rely on the current behaviour.

I think the current behaviour is:

 * "-serial none -serial something" is the same as
   "-serial something"
 * "-serial none" on its own disables the default serial
   device (the docs say it will "disable all serial ports"
   but I don't think that is correct...)
which amounts to "the only effectively useful use of
'-serial none' is to disable the default serial device"

and if we apply this patch:
 * "-serial none -serial something" has the sensible behaviour
   of "first serial port not connected/present, second serial
   port exists" (which of those you get depends on the machine
   model)
 * "-serial none" on its own has no behaviour change

So I think the only affected users would be anybody who
accidentally had an extra "-serial none" in their command
line that was previously being overridden by a later
"-serial" option. That doesn't seem very likely to me,
so I think I'd be in favour of making this change and
having something in the release notes about it.

Does anybody on the CC list have a different opinion /
think I've mis-analysed what the current code is doing ?

thanks
-- PMM