* Re: [PATCH] Fixed '-serial none' usage breaks following '-serial ...' usage
2024-01-15 16:14 ` Peter Maydell
@ 2024-01-16 7:33 ` Markus Armbruster
2024-01-16 17:34 ` Daniel P. Berrangé
2024-01-22 15:54 ` Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2024-01-16 7:33 UTC (permalink / raw)
To: Peter Maydell
Cc: Bohdan Kostiv, qemu-devel, Paolo Bonzini, Daniel P. Berrange
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fixed '-serial none' usage breaks following '-serial ...' usage
2024-01-15 16:14 ` Peter Maydell
2024-01-16 7:33 ` Markus Armbruster
@ 2024-01-16 17:34 ` Daniel P. Berrangé
2024-01-22 15:54 ` Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2024-01-16 17:34 UTC (permalink / raw)
To: Peter Maydell; +Cc: Bohdan Kostiv, qemu-devel, Markus Armbruster, Paolo Bonzini
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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fixed '-serial none' usage breaks following '-serial ...' usage
2024-01-15 16:14 ` Peter Maydell
2024-01-16 7:33 ` Markus Armbruster
2024-01-16 17:34 ` Daniel P. Berrangé
@ 2024-01-22 15:54 ` Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2024-01-22 15:54 UTC (permalink / raw)
To: Bohdan Kostiv
Cc: qemu-devel, Markus Armbruster, Paolo Bonzini, Daniel P. Berrange
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
^ permalink raw reply [flat|nested] 6+ messages in thread