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.