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

[Cao jin]

On 12/08/2015 11:01 PM, Markus Armbruster wrote:
> Cao jin <address@hidden> 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