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

[Daniel P . Berrangé]

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