[Qemu-devel] [PATCH for-2.12] chardev/char-fe: Allow NULL chardev in qemu_chr_fe_init()

[Qemu-devel] [PATCH for-2.12] chardev/char-fe: Allow NULL chardev in qemu_chr_fe_init()

[Peter Maydell]

All the functions in char-fe.c handle the CharBackend
having a NULL Chardev pointer, which means that the
backend exists but is not connected to anything. The
exception is qemu_chr_fe_init(), which will crash if
passed a NULL Chardev pointer argument. This can happen
for various boards if they're started with 'nodefaults':
 arm-softmmu/qemu-system-arm -S -nodefaults -M cubieboard
 riscv32-softmmu/qemu-system-riscv32 -nodefaults -M sifive_e

Make qemu_chr_fe_init() accept a NULL chardev. This allows
UART models to handle NULL chardev properties without
generally needing to special case them or to manually
create a NullChardev.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is my proposal for fixing the crashes with -nodefaults.
I think we should also change hw/char/serial.c to accept
a NULL Chardev rather than treating it as an error
( "Can't create serial device, empty char device"), and
similarly for any other devices that currently error out
on NULL chardevs. I'd rather postpone that part til 2.13, though.


 chardev/char-fe.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 392db78b13..b1f228e8b5 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -198,19 +198,21 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
 {
     int tag = 0;
 
-    if (CHARDEV_IS_MUX(s)) {
-        MuxChardev *d = MUX_CHARDEV(s);
+    if (s) {
+        if (CHARDEV_IS_MUX(s)) {
+            MuxChardev *d = MUX_CHARDEV(s);
 
-        if (d->mux_cnt >= MAX_MUX) {
+            if (d->mux_cnt >= MAX_MUX) {
+                goto unavailable;
+            }
+
+            d->backends[d->mux_cnt] = b;
+            tag = d->mux_cnt++;
+        } else if (s->be) {
             goto unavailable;
+        } else {
+            s->be = b;
         }
-
-        d->backends[d->mux_cnt] = b;
-        tag = d->mux_cnt++;
-    } else if (s->be) {
-        goto unavailable;
-    } else {
-        s->be = b;
     }
 
     b->fe_open = false;
-- 
2.16.2

Re: [Qemu-devel] [PATCH for-2.12] chardev/char-fe: Allow NULL chardev in qemu_chr_fe_init()

[Thomas Huth]

On 23.03.2018 16:29, Peter Maydell wrote:
> All the functions in char-fe.c handle the CharBackend
> having a NULL Chardev pointer, which means that the
> backend exists but is not connected to anything. The
> exception is qemu_chr_fe_init(), which will crash if
> passed a NULL Chardev pointer argument. This can happen
> for various boards if they're started with 'nodefaults':
>  arm-softmmu/qemu-system-arm -S -nodefaults -M cubieboard
>  riscv32-softmmu/qemu-system-riscv32 -nodefaults -M sifive_e
> 
> Make qemu_chr_fe_init() accept a NULL chardev. This allows
> UART models to handle NULL chardev properties without
> generally needing to special case them or to manually
> create a NullChardev.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is my proposal for fixing the crashes with -nodefaults.
> I think we should also change hw/char/serial.c to accept
> a NULL Chardev rather than treating it as an error
> ( "Can't create serial device, empty char device"), and
> similarly for any other devices that currently error out
> on NULL chardevs. I'd rather postpone that part til 2.13, though.

Sounds like a good plan.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH for-2.12] chardev/char-fe: Allow NULL chardev in qemu_chr_fe_init()

[Paolo Bonzini]

On 23/03/2018 16:29, Peter Maydell wrote:
> All the functions in char-fe.c handle the CharBackend
> having a NULL Chardev pointer, which means that the
> backend exists but is not connected to anything. The
> exception is qemu_chr_fe_init(), which will crash if
> passed a NULL Chardev pointer argument. This can happen
> for various boards if they're started with 'nodefaults':
>  arm-softmmu/qemu-system-arm -S -nodefaults -M cubieboard
>  riscv32-softmmu/qemu-system-riscv32 -nodefaults -M sifive_e
> 
> Make qemu_chr_fe_init() accept a NULL chardev. This allows
> UART models to handle NULL chardev properties without
> generally needing to special case them or to manually
> create a NullChardev.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is my proposal for fixing the crashes with -nodefaults.
> I think we should also change hw/char/serial.c to accept
> a NULL Chardev rather than treating it as an error
> ( "Can't create serial device, empty char device"), and
> similarly for any other devices that currently error out
> on NULL chardevs. I'd rather postpone that part til 2.13, though.
> 
> 
>  chardev/char-fe.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 392db78b13..b1f228e8b5 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -198,19 +198,21 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>  {
>      int tag = 0;
>  
> -    if (CHARDEV_IS_MUX(s)) {
> -        MuxChardev *d = MUX_CHARDEV(s);
> +    if (s) {
> +        if (CHARDEV_IS_MUX(s)) {
> +            MuxChardev *d = MUX_CHARDEV(s);
>  
> -        if (d->mux_cnt >= MAX_MUX) {
> +            if (d->mux_cnt >= MAX_MUX) {
> +                goto unavailable;
> +            }
> +
> +            d->backends[d->mux_cnt] = b;
> +            tag = d->mux_cnt++;
> +        } else if (s->be) {
>              goto unavailable;
> +        } else {
> +            s->be = b;
>          }
> -
> -        d->backends[d->mux_cnt] = b;
> -        tag = d->mux_cnt++;
> -    } else if (s->be) {
> -        goto unavailable;
> -    } else {
> -        s->be = b;
>      }
>  
>      b->fe_open = false;
> 

Queued, thanks.

Paolo