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

[Peter Maydell]

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