| Message ID | 1364292483-16564-8-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;
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(-)