Re: [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections

[Hans de Goede]

Hi,

On 03/27/2013 04:11 PM, Paolo Bonzini wrote:

<snip>

> > diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> > index 5e012e9..d8e9d63 100644
> > --- a/backends/rng-egd.c
> > +++ b/backends/rng-egd.c
> > @@ -149,6 +149,12 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
> >           return;
> >       }
> >
> > +    if (s->chr->avail_connections < 1) {
> > +        error_set(errp, QERR_DEVICE_IN_USE, s->chr_name);
> > +        return;
> > +    }
> > +    s->chr->avail_connections--;
> > +
> >       /* FIXME we should resubmit pending requests when the CDS reconnects. */
> >       qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
> >                             NULL, s);
> > @@ -191,6 +197,7 @@ static void rng_egd_finalize(Object *obj)
> >
> >       if (s->chr) {
> >           qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> > +        s->chr->avail_connections++;
> >       }
> >
> >       g_free(s->chr_name);
>
> Ok, but please create wrappers for these (e.g. qemu_chr_be_start/stop
> and qemu_chr_be_start_nofail) and use them throughout.

That would be fe_start fe_stop, ack otherwise.

> > diff --git a/gdbstub.c b/gdbstub.c
> > index a666cb5..83267e0 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -3025,6 +3025,7 @@ int gdbserver_start(const char *device)
> >           if (!chr)
> >               return -1;
> >
> > +        chr->avail_connections--;
> >           qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
> >                                 gdb_chr_event, NULL);
> >       }
>
> Ok.
>
> > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> > index 7467cca..df4b458 100644
> > --- a/hw/arm/pxa2xx.c
> > +++ b/hw/arm/pxa2xx.c
> > @@ -1981,9 +1981,16 @@ static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion 
> > *sysmem,
> >       memory_region_init_io(&s->iomem, &pxa2xx_fir_ops, s, "pxa2xx-fir", 
> > 0x1000);
> >       memory_region_add_subregion(sysmem, base, &s->iomem);
> >
> > -    if (chr)
> > +    if (chr) {
> > +        if (chr->avail_connections < 1) {
> > +            fprintf(stderr, "pxa2xx_fir_init error chardev %s already used\n",
> > +                    chr->label);
> > +            exit(1);
> > +        }
> > +        chr->avail_connections--;
> >           qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
> >                           pxa2xx_fir_rx, pxa2xx_fir_event, s);
> > +    }
> >
> >       register_savevm(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
> >                       pxa2xx_fir_load, s);
>
> Errors won't be reported, because serial_hds[] will always create its
> own CharDriverState and avail_connections will always be 1.  Use a
> wrapper and the code can ignore this.

Unless some smartass adds, ie: -mon chardev=serial0 to the cmdline, then an
error will be reported.

I'll respin the patch taking your comments into account.

Regards,

Hans