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

[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.

>>>
>>>          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