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

[Philippe Mathieu-Daudé]

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>