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

The patch fixes the issue.

Best regards,
Bohdan
From ee8023f3d466c3a5ad9cb9d117e11501c44da377 Mon Sep 17 00:00:00 2001
From: Bohdan Kostiv <bogdan.kostiv@gmail.com>
Date: Wed, 10 Jan 2024 11:09:11 +0400
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'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

On Mon, 15 Jan 2024 at 16:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> (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);
> >

While I was testing this patch, I discovered that it has a bug:
if you run 'qemu-system-x86_64 -serial none' it now crashes.
This happens because the serial_hd() function assumes that
serial_hds points to enough memory for num_serial_hds
pointers, and now we are increasing num_serial_hds but
skipping the g_renew() that enlarges the array.

I'll send a patch which gets that part right and also has
an expanded commit message which mentions some of the
things we've discussed in this thread.

thanks
-- PMM

On Mon, Jan 15, 2024 at 04:14:30PM +0000, Peter Maydell wrote:
> (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.

If we don't apply this patch, then the valid use case of the reporter
here is impossible to achieve. We would have to invent a new syntax
to enable what 'serial none -serial something' should have already
been doing.  There is nothing users can do without us applying a fix
of some kind, either as proposed in the patch here, or something that
is functionally identical with different cli.


If we do apply this patch and someone was (mistakenly) relying on
'-serial none -serial something' being functionally equivalent to
'-serial something', they have an easy fix. They can just remove
the redundant '-serial none' they have and this works with any
QEMU they might see, whether ancient, current or future.


On balance, I think it is the right tradeoff to apply the proposed
patch, and accept the small risk of breaking someone who was
mistakenly relying on the broken behaviour, since the impact to
those people should be small.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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.

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

Thanks for this patch. I'll have a think about whether it's
the right fix next week. In the meantime, I suspect you'll find
that if you use "-serial null -serial null -serial mon:stdio" in
your command line example you'll find that does what you're aiming for.

("-serial none" means "don't create a serial port device", whereas
"-serial null" means "create a serial port device, but have its
input and output go to nowhere".)

thanks
-- PMM