From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xmazp-0003Oj-Ki for qemu-devel@nongnu.org; Thu, 06 Nov 2014 23:18:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xmazh-00084O-Sk for qemu-devel@nongnu.org; Thu, 06 Nov 2014 23:18:01 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:42372) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xmazh-00083l-JG for qemu-devel@nongnu.org; Thu, 06 Nov 2014 23:17:53 -0500 Received: from epcpsbgm2.samsung.com (epcpsbgm2 [203.254.230.27]) by mailout2.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0NEN00JBJILRQ930@mailout2.samsung.com> for qemu-devel@nongnu.org; Fri, 07 Nov 2014 13:17:51 +0900 (KST) From: SeokYeon Hwang References: <1415182311-13799-1-git-send-email-syeon.hwang@samsung.com> <20141105124609.GA4354@redhat.com> <87lhnpn5y1.fsf@blackfin.pond.sub.org> <545A23A7.7090401@redhat.com> <20141105132815.GA4884@redhat.com> <545A3A61.50001@redhat.com> <045901cff969$02203060$06609120$@samsung.com> <20141106092355.GA15186@redhat.com> In-reply-to: <20141106092355.GA15186@redhat.com> Date: Fri, 07 Nov 2014 13:17:53 +0900 Message-id: <077901cffa41$cd7467d0$685d3770$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7bit Content-language: ko Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "'Michael S. Tsirkin'" Cc: 'Paolo Bonzini' , 'Markus Armbruster' , qemu-devel@nongnu.org > -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Thursday, November 06, 2014 6:24 PM > To: SeokYeon Hwang > Cc: 'Paolo Bonzini'; 'Markus Armbruster'; qemu-devel@nongnu.org > Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling > between pci_qdev_init() and qdev > > On Thu, Nov 06, 2014 at 11:26:01AM +0900, SeokYeon Hwang wrote: > > > > > > > -----Original Message----- > > > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of > > > Paolo Bonzini > > > Sent: Wednesday, November 05, 2014 11:55 PM > > > To: Michael S. Tsirkin > > > Cc: Markus Armbruster; SeokYeon Hwang; qemu-devel@nongnu.org > > > Subject: Re: [Qemu-devel] [PATCH] pci: fixed mismatch of > > > error-handling between pci_qdev_init() and qdev > > > > > > > > > > > > On 05/11/2014 14:28, Michael S. Tsirkin wrote: > > > > > I think bypassing the question by converting to realize makes > > > > > the most sense... > > > > > > > > I'm fine with doing that but Markus's patches wouldn't yet have > > > > solved the problem by themselves since init is still around, right? > > > > > > > > This probably means fixing this bug can't justify merging the > > > > realize patchset after freeze. > > > > > > Yes, I agree. I meant that the API is not very well defined. I > > > would handle everything else on a case-by-case basis, by reviewing > > > each init function that is converted to realize. > > > > > > Since the patch was for an out-of-tree device, it can wait for 2.3 > anyway. > > > > > > Paolo > > > > I cannot fully understand your conversation. > > But, I think this patch is still worth before all 'init()' convert to > > 'realize()'. > > Moreover, It has no side effect at all. > > > > Thanks. > > > > The root cause is API misuse: functions that return int should return a > negative code on failure, either 0 or >= 0 on success. > In rare cases, we use int as bool, so 0 on failure, 1 on success. > > Your device returned 1 on failure, this broke things. > So don't do this then :) > > The question would be: are there existing devices that return a positive > return code on init. If there are, it's a bug, but the best fix might be > your patch - easier that fixing many devices. > > If there aren't, the patch isn't needed. > > -- > MST You are right. Yes, it is API misuse. So we had fixed the device that return positive value on failure. But is that all right ?? I don't think so, because this logic is obviously wrong. 'pci_qdev_init()' should return -1 (not rc) on failure or should check 'rc < 0' not 'rc != 0'. That is what I wanted to fix. But if 2.2 comes with all "realized" devices, if there is no "init" devices, then this patch isn't needed. Thanks