Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init()

[Cao jin <caoj.fnst@cn.fujitsu.com>]

Hi Marcel,
    Thanks for your quick review for this big fat patch:)
    please see my comments inline.

On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote:
> On 03/28/2016 01:44 PM, Cao jin wrote:

>>
>> -#define VMXNET3_USE_64BIT         (true)
>> -#define VMXNET3_PER_VECTOR_MASK   (false)
>> -
>> -static bool
>> -vmxnet3_init_msi(VMXNET3State *s)
>> -{
>> -    PCIDevice *d = PCI_DEVICE(s);
>> -    int res;
>> -
>> -    res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>> -                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>> -    if (0 > res) {
>> -        VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> -        s->msi_used = false;
>> -    } else {
>> -        s->msi_used = true;
>> -    }
>> -
>> -    return s->msi_used;
>> -}
>> -
>>   static void
>>   vmxnet3_cleanup_msi(VMXNET3State *s)
>>   {
>> @@ -2271,6 +2250,10 @@ static uint8_t
>> *vmxnet3_device_serial_num(VMXNET3State *s)
>>       return dsnp;
>>   }
>>
>> +
>> +#define VMXNET3_USE_64BIT         (true)
>> +#define VMXNET3_PER_VECTOR_MASK   (false)
>> +
>>   static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>       DeviceState *dev = DEVICE(pci_dev);
>> @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice
>> *pci_dev, Error **errp)
>>
>>       VMW_CBPRN("Starting init...");
>>
>> +    msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>> +             VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
>> +    if (*errp) {
>> +        error_report_err(*errp);
>> +        *errp = NULL;
>
> You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI,
> error %d", res),
> but you replace with one of yourself. If the vmxnet maintainers have
> nothing against,
> I suppose is OK.
>
> Then, you don't propagate the error because you don't want realize
> to fail, so you report it and NULL it. I suppose we should have
> a "warning only" error type so the reporting party can decide to issue a
> warning
> or to fail, but is out of the scope of this patch, of course.
>

Yes, I should add more hint message. I don`t quite understand about:

/have a "warning only" error type so the reporting party can decide to 
issue a  warning or to fail/

Do you mean still using VMW_WRPRN or using error_append_hint? It seems 
VMW_WRPRN should only work in debug time? and if user should see this 
error message, should use error_report_err ?

>> -    if (!vmxnet3_init_msi(s)) {
>> -        VMW_WRPRN("Failed to initialize MSI, configuration is
>> inconsistent.");
>
> Here again you remove an existent debug warning, maybe you should keep it.
>
>> -    }
>> -
>>       vmxnet3_net_init(s);
>>

>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>> b/hw/pci-bridge/pci_bridge_dev.c
>> index 862a236..07c7bf8 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -52,6 +52,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>       PCIBridge *br = PCI_BRIDGE(dev);
>>       PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>       int err;
>> +    Error *local_err = NULL;
>
> You use both local_err and err for local error names. It doesn't really
> matter
> the name, but please be consistent. I personally like local_error but is
> up to you, of course.
>

Yes, I prefer consistent way, but here, you see there is a "int err", so 
I thought two different variants using same name maybe not so good for 
reading code?  what would you choose?(I like local_err at first because 
it is easy to understand for newbie, but when I get familiar with error 
handling, I turn to like err, just because it is shorter:p)

>>
>>       pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>
>> @@ -67,17 +68,21 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>           /* MSI is not applicable without SHPC */
>>           bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
>>       }
>> +
>>       err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>       if (err) {
>>           goto slotid_error;
>>       }
>> +
>
> You have several new lines (before and after this comment) that have
> nothing
> to do with the patch. I suggest putting them into a trivial separate patch.
>

see what I was told before:
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html

Actually I am ok with both sides. I just stop sending coding style patch 
since then:)

I don`t know, coding style & indentation patch really matters or is just 
a personal taste thing?

>> +++ b/hw/scsi/mptsas.c
>> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev,
>> Error **errp)
>>   {
>>       DeviceState *d = DEVICE(dev);
>>       MPTSASState *s = MPT_SAS(dev);
>> +    Error *err = NULL;
>>
>>       dev->config[PCI_LATENCY_TIMER] = 0;
>>       dev->config[PCI_INTERRUPT_PIN] = 0x01;
>>
>> +    if (s->msi_available) {
>> +        if (msi_init(dev, 0, 1, true, false, &err) >= 0) {
>> +            s->msi_in_use = true;
>> +        }
>> +        else {
>> +            error_append_hint(&err, "MSI init fail, will use INTx
>> instead");
>
> The hint should end with a new line: "\n".
>
>> +            error_report_err(err);
>
> You should not report the error if the fucntion has an **errp parameter.
>

it will use INTx even if msi_init fail, so it should not break realize. 
But I think user should know it if something wrong happened there，so I 
use a local *err* to report error message, while don`t touch **errp. Is 
there any better way?

>

For all the other comments, will fix them in next version.
-- 
Yours Sincerely,

Cao jin