Re: [Qemu-devel] [PATCH for-4.0 v9 09/16] qemu_thread: supplement error handling for pci_edu_realize

[Fei Li]

在 2019/1/15 下午8:55, Markus Armbruster 写道:
> Fei Li <address@hidden> writes:
>
> > 在 2019/1/14 下午8:36, Markus Armbruster 写道:
> > > Fei Li <address@hidden> writes:
> > >
> > > > Just to make sure about how to do the cleanup. I notice that in 
> > > > device_set_realized(),
> > > > the current code does not call "dc->unrealize(dev, NULL);" when dc->realize() 
> > > > fails.
> > Sorry that I am still uncertain.. I guess the code below I pasted was
> > misleading,
> > actually I want to stress the *dc->unrealize() is not called when
> > dc->realize() fails*
> > and the incomplete below "goto fail" does not include the dc->unrealize(),
> > but instead the dc->unrealize() is included in later
> > child_realize_fail: & post_realize_fail:.
> >
> >
> > Emm, IMHO, I think when dc->realize() fails, the dc->unrealize() is
> > either should be
> > called in the common function: device_set_realized() in a unified way,
> > that is
> >
> >          if (local_err != NULL) {
> > +          if (dc->unrealize) {
> > +              dc->unrealize(dev, local_err);
> > +          }
> >              goto fail;
> >          }
> >
> > or do the unrealize() locally for each device earlier when
> > dc->realize() fails.
> >
> > But I checked several dc->realize() function, they did not call unrealize()
> > when fails. Besides, it may mean verbose code if unrealize() locally.
> > Thus I think the above way is the right way to do the cleanup when
> > realize() fails.
> The realize() method is specified to either succeed completely or fail
> completely, i.e. fail and do nothing.  The "either succeed completely or
> fail completely" aspect of the specification is sane and perfectly
> ordinary.
>
> How a concrete implementation accomplishes "fail completely" is up to
> the implementation.
>
> An implementation may choose to structure its FOO_realize() and
> FOO_unrealize() in a way that lets FOO_realize() call FOO_unrealize() to
> clean up on failure.
>
> An implementation may also choose to clean up differently.
>
> This freedom of choice is by design.
>
> Changing the specification now would involve auditing and updating all
> realize() and unrealize() methods.  Not going to happen without an
> extremely compelling reason.

Ok, now I see. Thanks for the detail explanation. :)

Will add the cleanup in the next version.

Have a nice day
Fei
> > > >           if (dc->realize) {
> > > >               dc->realize(dev, &local_err);
> > > >           }
> > > >
> > > >           if (local_err != NULL) {
> > > >               goto fail;
> > > >           }
> > > >
> > > > Is this on purpose? (Maybe due to some devices' realize() do their own cleanup
> > > > when fails? Sorry for the unsure, it is such a common function that I did not
> > > > check all. :( ) Or else, I prefer to do the cleanup in a unified manner, e.g. call 
> > > > "dc->unrealize(dev, NULL);" which is the pci_qdev_unrealize() for pci devices.
> > > Yes, this is on purpose.
> > >
> > > When a realize() method fails, it must revert everything it has done so
> > > far.  Results in sane "either succeed completely, or fail and do
> > > nothing" semantics.
> > Have a nice day, thanks
> >
> > Fei