From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55817) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKU0Q-0001Ik-JD for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:33:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKU0N-0005g8-O8 for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:33:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKTuI-0003lc-BC for qemu-devel@nongnu.org; Tue, 26 Mar 2013 09:27:18 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2QDRHdV010882 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Mar 2013 09:27:17 -0400 Message-ID: <5151A301.30503@redhat.com> Date: Tue, 26 Mar 2013 14:30:41 +0100 From: Hans de Goede 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> In-Reply-To: <51519DA6.3070306@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Paolo Bonzini Cc: Amit Shah , qemu-devel@nongnu.org, Gerd Hoffmann 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? 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