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

[Hans de Goede]

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?

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