Patchwork [07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

login
register
mail settings
Submitter Hans de Goede
Date March 26, 2013, 10:07 a.m.
Message ID <1364292483-16564-8-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/231153/
State New
Headers show

Comments

Hans de Goede - March 26, 2013, 10:07 a.m.
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 <hdegoede@redhat.com>
---
 hw/qdev-properties-system.c | 6 ++++--
 qemu-char.c                 | 2 --
 2 files changed, 4 insertions(+), 4 deletions(-)
Paolo Bonzini - March 26, 2013, 1:07 p.m.
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 <hdegoede@redhat.com>
> ---
>  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?

Paolo
Hans de Goede - March 26, 2013, 1:30 p.m.
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 <hdegoede@redhat.com>
>> ---
>>   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?

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 ?

Regards,

Hans
Paolo Bonzini - March 26, 2013, 1:50 p.m.
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 <hdegoede@redhat.com>
>>> ---
>>>   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
Hans de Goede - March 27, 2013, 2:09 p.m.
Hi,

On 03/26/2013 02:50 PM, Paolo Bonzini wrote:

<snip>

>> 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.

Yes on x86 maybe, but a lot of the other serial-port emulations are
still using serial_hds directly, making proper avail_connections tracking
a pain.

Anyways I've audited all frontends now, fixing things where necessary,
and where possible in a generic way.

I'll send a patch for this right after this mail.

Regards,

Hans
Paolo Bonzini - March 27, 2013, 2:58 p.m.
Il 27/03/2013 15:09, Hans de Goede ha scritto:
> Hi,
> 
> On 03/26/2013 02:50 PM, Paolo Bonzini wrote:
> 
> <snip>
> 
>>> 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.
> 
> Yes on x86 maybe, but a lot of the other serial-port emulations are
> still using serial_hds directly, making proper avail_connections tracking
> a pain.

serial_hds is still passed to most devices via a chardev qdev property.
 See for example sparc/leon3.c, which uses grlib_apbuart_create and that
function sets the chardev.

Luckily there are very few UART implementations, most boards use the
8250/16550, hence this is even true of boards that are generally not
qdev-ified (like OMAP).  There are exceptions, like mcf_uart.c and
bt-hci-csr.c.

Paolo

> Anyways I've audited all frontends now, fixing things where necessary,
> and where possible in a generic way.
> 
> I'll send a patch for this right after this mail.
> 
> Regards,
> 
> Hans
Hans de Goede - March 27, 2013, 3:16 p.m.
Hi,

On 03/27/2013 03:58 PM, Paolo Bonzini wrote:
> Il 27/03/2013 15:09, Hans de Goede ha scritto:
>> Hi,
>>
>> On 03/26/2013 02:50 PM, Paolo Bonzini wrote:
>>
>> <snip>
>>
>>>> 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.
>>
>> Yes on x86 maybe, but a lot of the other serial-port emulations are
>> still using serial_hds directly, making proper avail_connections tracking
>> a pain.
>
> serial_hds is still passed to most devices via a chardev qdev property.

Most, yes but not all, which is why I wrote "using serial_hds *directly*",
anyways see the patch which I send a while back which tries to deal with
all the *direct* serial_hds users, as well as with the monitor, and some
code which does chardev creation completely on its own.

Regards,

Hans

Patch

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;