Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 12/08/2015 11:01 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> 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