From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33217) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKUGW-0000e0-VL for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:50:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKUGR-0003t2-58 for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:50:16 -0400 Received: from mail-ea0-x22e.google.com ([2a00:1450:4013:c01::22e]:33875) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKUGQ-0003s2-Uf for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:50:11 -0400 Received: by mail-ea0-f174.google.com with SMTP id m14so772980eaj.33 for ; Tue, 26 Mar 2013 06:50:10 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5151A78D.5010708@redhat.com> Date: Tue, 26 Mar 2013 14:50:05 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1364292483-16564-1-git-send-email-hdegoede@redhat.com> <1364292483-16564-8-git-send-email-hdegoede@redhat.com> <51519DA6.3070306@redhat.com> <5151A301.30503@redhat.com> In-Reply-To: <5151A301.30503@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hans de Goede Cc: Amit Shah , qemu-devel@nongnu.org, Gerd Hoffmann 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 >>> --- >>> 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