Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Paolo Bonzini]

Il 26/03/2013 14:30, Hans de Goede ha scritto:
> Hi,
> 
> On 03/26/2013 02:07 PM, Paolo Bonzini wrote:
>> Il 26/03/2013 11:07, Hans de Goede ha scritto:
>>> The decrement of avail_connections is done in qdev-properties-system
>>> move
>>> the increment there too for proper balancing of the calls.
>>>
>>> Signed-off-by: Hans de Goede <address@hidden>
>>> ---
>>>   hw/qdev-properties-system.c | 6 ++++--
>>>   qemu-char.c                 | 2 --
>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
>>> index 8795144..12a87d5 100644
>>> --- a/hw/qdev-properties-system.c
>>> +++ b/hw/qdev-properties-system.c
>>> @@ -136,9 +136,11 @@ static void release_chr(Object *obj, const char
>>> *name, void *opaque)
>>>       DeviceState *dev = DEVICE(obj);
>>>       Property *prop = opaque;
>>>       CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>>> +    CharDriverState *chr = *ptr;
>>>
>>> -    if (*ptr) {
>>> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
>>> +    if (chr) {
>>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>> +        ++chr->avail_connections;
>>>       }
>>>   }
>>>
>>> diff --git a/qemu-char.c b/qemu-char.c
>>> index 8a66627..368e7f5 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -197,8 +197,6 @@ void qemu_chr_add_handlers(CharDriverState *s,
>>>       int fe_open;
>>>
>>>       if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>>> -        /* chr driver being released. */
>>> -        ++s->avail_connections;
>>>           fe_open = 0;
>>>       } else {
>>>           fe_open = 1;
>>>
>>
>> I think this is still wrong (though better than before this patch).
>> This is still not giving an error:
>>
>>     qemu-kvm \
>>        -chardev stdio,id=foo -device isa-serial,chardev=foo \
>>        -mon chardev=foo
>>
>> because other users of -chardev (monitor and rng-egd), are not
>> decrementing avail_connections.  Can you look at it in a follow-up?
> 
> I know, I ended up writing this patch mostly as a side-effect.
> 
> I can put further fixing this on my TODO list but first I've some
> questions about this which need answering:
> 
> 1) For most problematic devices, the proper fix would be to make them
> use a chardev qdev property for there chardev usage, and then this
> would be automatically fixed, agreed?

At least on x86, all devices already use a chardev qdev property.

> 2) For some this may not fly and a manual inc / dec of avail_connections
> is necessary, ie the monitor, agreed?
> 
> 3) One weird case which I encountered when working on this I noticed
> that backends/rng-egd.c has its chardev as a string qdev-property, rather
> then as a chardev qdev-property and then it does a qemu_chr_find itself,
> is this intentional, iow is there some reason having it as a
> chardev qdev-property does not work ?

The infrastructure for chardev qdev properties right now is only used
within devices.  The right thing to do would be to make chardevs QOM
objects.  Then you do not need any special code, just make chardevs QOM
links.

Paolo