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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> (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'.

I was about to ask whether this is a regression, but then ...

> 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.

... saw you already showed it is.

>                                             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...)

Goes back all the way to the commit that introduced "none": c03b0f0fd86
(allow disabling of serial or parallel devices (Stefan Weil)), v0.9.0.

> which amounts to "the only effectively useful use of
> '-serial none' is to disable the default serial device"

Yes.

> 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)

Is this the behavior before commit 998bbd74b9d?

>  * "-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 ?

I'm leaning towards agreeing with you.  A bit more heavily if the change
restores original behavior.

Your analysis should be worked into the commit message, though.