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

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/30/2016 07:10 AM, Cao jin wrote:
> 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.
>>

Hi,

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

No, it is not related to VMW_WRPRN. On this subject, those are debugging warnings
and we should keep them the same as before.

About the "warning only" error type: if an error is returned, the caller
assumes that the initialization failed and it will return with failure. That means
that you cannot return a non-null error if you want the process to finish OK.

This is why you had to:
    -  report the error (even if this function should not be a reporter because it receives an Error parameter)
    -  empty the error: so why use error at all, right?

My point is if the caller had a way to check what kind of error this is
and act accordingly, it would be nicer. But again, this is out of the scope of this patch, only some thoughts.

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

Indeed is a little confusing, this is why I prefer "local_error" because it is used widely
and became a familiar pattern.

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

Oh, I hate when it happens to me, tho different opinions, how can you deal with that, right ? :)

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

Coding style and indentation *are important* IMHO.

For this case, what I would do is put the new lines and comments in a separate patch,\
but send it with *the same* series, not through trivial list, saying something like:
"fixed some code style problems while resolving the ... problem".

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

Same problem as discussed before, Markus do you have any idea how to solve this elegantly?
CC: Markus Armbruster <armbru@redhat.com>

Thanks,
Marcel

>
>>
>
> For all the other comments, will fix them in next version.