Peter Maydell <peter.mayd...@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.kos...@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.kos...@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.
