Re: [Qemu-devel] [PATCH v2 2/2] hw/pci-bridge: Convert pxb initialization functions to Error

[Markus Armbruster]

Cao jin <address@hidden> writes:

> On 03/23/2016 03:26 PM, Wei Jiangang wrote:
>
>>
>> -static int pxb_dev_init_common(PCIDevice *dev, bool pcie)
>> +static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>>   {
>>       PXBDev *pxb = convert_to_pxb(dev);
>>       DeviceState *ds, *bds = NULL;
>>       PCIBus *bus;
>>       const char *dev_name = NULL;
>> +    Error *local_err = NULL;
>>
>
> the preferred style I think, is /*err/

Both styles are in use.  I use err myself, but local_err is okay, too.

>>       if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>>           pxb->numa_node >= nb_numa_nodes) {
>> -        error_report("Illegal numa node %d.", pxb->numa_node);
>> -        return -EINVAL;
>> +        error_setg(errp, "Illegal numa node %d", pxb->numa_node);
>> +        return;
>
> since we have local /*err/ to avoid null /**errp/ venture, I guess it
> should be used here too.

No, this is just fine.

If errp is null, error_setg() does nothing, which is exactly what we
want.

However:

>>       }
>>
>>       if (dev->qdev.id && *dev->qdev.id) {
>> @@ -248,7 +244,9 @@ static int pxb_dev_init_common(PCIDevice *dev, bool pcie)
>>
>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>>
>> -    if (pxb_register_bus(dev, bus)) {
>> +    pxb_register_bus(dev, bus, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>           goto err_register_bus;
>>       }
>>

When we need to find out whether the callee set an error, we can't use
(possibly null) errp, because *errp isn't safe.

[...]