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

[Hans de Goede <hdegoede@redhat.com>]

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