Re: [PATCH v3 2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 11/11/22 13:32, Klaus Jensen wrote:
> On Nov 11 12:40, Philippe Mathieu-Daudé wrote:
>> On 10/11/22 23:08, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Replace the local Error variable with errp and ERRP_GUARD() and change
>>> the return value to bool.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>>    hw/nvme/ctrl.c | 23 ++++++++++-------------
>>>    1 file changed, 10 insertions(+), 13 deletions(-)
>>
>>
>>> @@ -7388,14 +7387,12 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>>>        }
>>>        ret = msix_init(pci_dev, n->params.msix_qsize,
>>>                        &n->bar0, 0, msix_table_offset,
>>> -                    &n->bar0, 0, msix_pba_offset, 0, &err);
>>> -    if (ret < 0) {
>>> -        if (ret == -ENOTSUP) {
>>> -            warn_report_err(err);
>>> -        } else {
>>> -            error_propagate(errp, err);
>>> -            return ret;
>>> -        }
>>> +                    &n->bar0, 0, msix_pba_offset, 0, errp);
>>> +    if (ret == -ENOTSUP) {
>>> +        warn_report_err(*errp);
>>
>> Why only report ENOTSUP in particular?
>>
> 
> Because the error is beneign (it's just a notice that MSI-X isnt
> available on the platform).
> 
>>> +        *errp = NULL;
>>> +    } else if (ret < 0) {
>>> +        return false;
>>
>> Is that normal to ignore:
>>
>> -   error_setg(errp, "The number of MSI-X vectors is invalid");
>>      return -EINVAL;
>>
>> -   error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
>>                       " or don't align");
>>      return -EINVAL;
>>
>> Or possible future error added in msix_init()?
> 
> It is not ignored, it is passed up to the caller. On any other error,
> returning false will cause device realization to fail and the error
> (i.e. invalid vectors or overlap) be reported.

Indeed, I didn't review carefully enough.

Maybe in the first case explicit with /* Convert the error as a simple 
warning */ and in the second /* Propagate to caller */.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>