Re: [Qemu-devel] [PATCH] pci-bridge/i82801b11: Convert to realize

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On 27/05/2017 9:58, Mao Zhongyi wrote:
>>
>>
>> On 05/26/2017 10:08 PM, Marcel Apfelbaum wrote:
>>>
>>>
>>> On 26/05/2017 15:15, Mao Zhongyi wrote:
>>>> The pci-birdge device i82801b11 still implements the old
>>>> PCIDeviceClass .init() through i82801b11_bridge_init()
>>>> instead of the new .realize(). All devices need to be
>>>> converted to .realize(). So convert it and rename it to
>>>> i82801b11_bridge_realize().
>>>>
>>>> Signed-off-by: Mao Zhongyi <address@hidden>
>>>> ---
>>>>   hw/pci-bridge/i82801b11.c | 9 ++++-----
>>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>>>> index 2404e7e..dca3162 100644
>>>> --- a/hw/pci-bridge/i82801b11.c
>>>> +++ b/hw/pci-bridge/i82801b11.c
>>>> @@ -44,6 +44,7 @@
>>>>   #include "qemu/osdep.h"
>>>>   #include "hw/pci/pci.h"
>>>>   #include "hw/i386/ich9.h"
>>>> +#include "qapi/error.h"
>>>> /*****************************************************************************/
>>>> @@ -58,7 +59,7 @@ typedef struct I82801b11Bridge {
>>>>       /*< public >*/
>>>>   } I82801b11Bridge;
>>>>   -static int i82801b11_bridge_initfn(PCIDevice *d)
>>>> +static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>>>>   {
>>>>       int rc;
>>>>   @@ -70,12 +71,10 @@ static int i82801b11_bridge_initfn(PCIDevice *d)
>>>>           goto err_bridge;
>>>>       }
>>>>       pci_config_set_prog_interface(d->config,
>>>> PCI_CLASS_BRIDGE_PCI_INF_SUB);
>>>> -    return 0;
>>>> +    return;
>>>>     err_bridge:
>>>>       pci_bridge_exitfn(d);
>>>
>>> Hi,
>>>
>>> Since you move to realize, you may want to leverage the errp to add
>>> info on errors.
>>>
>>> Thanks,
>>> Marcel
>>
>>
>> Hi, Marcel
>>
>
> Hi!
>> Thanks for your quick reply and advice. In fact, I have considered
>> adding an error
>> message to errp when an error occurs. But I found that
>> pci_bridge_ssvid_init() has
>> reported a specific info when it fails. If the error is reported
>> here again, it seems
>> a bit more superfluous, so it's not adopted.
>
> I agree we don't want to see the error twice, but that means we may want
> to pass the error pointer to pci_bridge_ssvid_init and so on.

Yes.

Callees that report errors differently are a common issue when
converting a function to Error.  The correct solution is to convert
these callees to Error, too.

Sometimes, this is just too much to be practical.  Then a "shallow"
conversion can make sense as a stop-gap, even though it'll result in
clearly inferior error reporting.

At a glance, this one doesn't look impractical.

> One of the main things we achieve when moving to 'realize' is better
> error handling, so if we don't do that maybe is not worth it.

We need to convert all devices to realize() so we can get rid of the old
qdev methods.  But you're right in that we better convert them
correctly.

> You may need to do several changes you didn't intend to
> in order to do the error propagation right...

Yes.  An Error conversion can look simple at first, then balloon into a
multi-part series.

>
> Thanks,
> Marcel
>
>> Of course, output a readable error info
>> to make it easier for users is also a good choice. So, are you sure
>> you want to do
>> that?
>>
>> Look forward to your feedback and suggestion soon.
>>
>> Thanks a lot.
>> Mao

Any help with converting the remaining devices to realize() is much
appreciated.