From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6akX-0005kl-EX for qemu-devel@nongnu.org; Wed, 09 Dec 2015 04:09:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6akW-0000wI-C2 for qemu-devel@nongnu.org; Wed, 09 Dec 2015 04:09:25 -0500 References: <1449475725-16453-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1449475725-16453-2-git-send-email-caoj.fnst@cn.fujitsu.com> <87a8pmziil.fsf@blackfin.pond.sub.org> <5666D9EF.1090803@cn.fujitsu.com> <87d1uh6l25.fsf@blackfin.pond.sub.org> From: Cao jin Message-ID: <5667EC42.9060101@cn.fujitsu.com> Date: Wed, 9 Dec 2015 16:54:26 +0800 MIME-Version: 1.0 In-Reply-To: <87d1uh6l25.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: jiri@resnulli.us, qemu-block@nongnu.org, mst@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, alex.williamson@redhat.com, sfeldma@gmail.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com On 12/08/2015 11:01 PM, Markus Armbruster wrote: > Cao jin writes: > >> Hi Markus, >> I have to say, you really did a amazing review for this "trivial >> "patch, thanks a lot & really appreciate it:) > > Thanks! I'm afraid the problem you picked isn't trivial, but I hope > it's still simple enough to be a useful exercise to get you going with > the code. > [...] > > In theory, realize() should always fail cleanly. In practice, unclean > realize() failure doesn't matter when it's fatal anyway. Some devices > are only used where it's always fatal. See also "Error handling in > realize() methods" I just sent to the list; I hope we can come up with > some guidance on when shortcuts in realize() methods are tolerable. > Read it, really great background introduction to me:) [...] >> >> [*]Actually, here is my consideration: a device-realize function(take >> the following ioh3420 for example) will call many supporting functions >> like msi_init(), so I am planning, every supporting function goes into >> a patch first, then every "device convert to realize()" goes into a >> patch, otherwise, it may will be a big patch for the reviewer. That`s >> why I didn`t add Error ** param, and propagate it, and plan to do it >> in "convert to realize()" patch. But for now, I think this patch >> should at least be successfully compiled & won`t impact the existed >> things. >> >> Yes, it seems may have leaks when error happens, but will be fixed >> when the "convert to realize()" patch comes out. >> >> I am not sure whether the plan is ok, So, How do you think? > > If you don't want to propagate the error further in this patch, you need > to free it: > > if (0 > res) { > VMW_WRPRN("Failed to initialize MSI, error %d", res); > error_free(local_err); > s->msi_used = false; > > While there, you could improve the error message by printing > error_get_pretty(local_err)) instead of res, but I wouldn't bother, > since we'll have to replace it with error_propagate() anyway. > Agree. will fix it later. [...] >> >> Refer previous comment[*]: Was planning propagate it in "convert to >> realize()" patch later. > > Again, if you don't want to propagate the error further in this patch, > you need to free it. Actually, you have to report and free it; see > msi_init() below for why. The obvious way to do that: > > if (rc < 0) { > error_report_err(local_err); > goto err_bridge; > } > > By the way, I use err instead of local_err. Matter of taste. > Agree. will fix it later. [...] >>> >>> To assess the patch's impact, you need to compare before / after for >>> both failure modes and each caller. I suspect the result will promote >>> your patch from "prepare for realize" to "fix to catch and report >>> errors". > > Let's do that for ioh3420_initfn(). > > Before: > * when !msi_supported, ioh3420_initfn() fails silently > * when pci_add_capability() fails, we report with error_report_err(), > and ioh3420_initfn() fails > > After (your patch): > * when !msi_supported, ioh3420_initfn() fails silently > * when pci_add_capability2() fails, ioh3420_initfn() fails silently, > regression > > After (with ioh3420_initfn() calling error_report_err(), as per my > review): > * when !msi_supported, ioh3420_initfn() reports with error_report_err() > and fails, bug fix (mention in commit message) > * when pci_add_capability2() fails, reports with error_report_err() and > fails > > Do that for every caller of msi_init(), then summarize the patch's > impact change in the commit message. > great example for me, I Will try to do it. >> Thanks a lot for the direction:) but I still have a question: if I >> start off by per-device, then will modify every supporting function, >> and common supporting function will impact other device, so will need >> to convert other device together, and this will result in a huge patch >> contains converting of many devices and supporting functions, what do >> you think of it? > > A huge patch would be much harder to review. Limiting this patch to > just msi_init() is sensible. But the patch mustn't break things. Not > even if you plan to fix the breakage in later patches. > Agree. will try to undo the prior side effects and handle the error(like via error_report_err()) temporary[1] in next version. [1]all error will be propagated ultimately, for hot-pluggable device. > [...] > > > . > -- Yours Sincerely, Cao Jin