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

[Klaus Jensen]

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.

signature.asc
Description: PGP signature