[PATCH v3 0/2] hw/nvme: errp fixes

[PATCH v3 0/2] hw/nvme: errp fixes

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Fix a couple of invalid errp usages.

v3:
 - reword the commit message in patch 1
 - fix an embarrassing bug in patch 2

Klaus Jensen (2):
  hw/nvme: fix incorrect use of errp/local_err
  hw/nvme: cleanup error reporting in nvme_init_pci()

 hw/nvme/ctrl.c | 71 +++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)

-- 
2.38.1

[PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Remove an unnecessary local Error value in nvme_realize(). In the
process, change nvme_check_constraints() into returning a bool.

Finally, removing the local Error value also fixes a bug where an error
returned from nvme_init_subsys() would be lost.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index ac3885ce5079..a5c0a5fa6ce2 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7035,7 +7035,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
     },
 };
 
-static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
+static bool nvme_check_params(NvmeCtrl *n, Error **errp)
 {
     NvmeParams *params = &n->params;
 
@@ -7049,38 +7049,38 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
     if (n->namespace.blkconf.blk && n->subsys) {
         error_setg(errp, "subsystem support is unavailable with legacy "
                    "namespace ('drive' property)");
-        return;
+        return false;
     }
 
     if (params->max_ioqpairs < 1 ||
         params->max_ioqpairs > NVME_MAX_IOQPAIRS) {
         error_setg(errp, "max_ioqpairs must be between 1 and %d",
                    NVME_MAX_IOQPAIRS);
-        return;
+        return false;
     }
 
     if (params->msix_qsize < 1 ||
         params->msix_qsize > PCI_MSIX_FLAGS_QSIZE + 1) {
         error_setg(errp, "msix_qsize must be between 1 and %d",
                    PCI_MSIX_FLAGS_QSIZE + 1);
-        return;
+        return false;
     }
 
     if (!params->serial) {
         error_setg(errp, "serial property not set");
-        return;
+        return false;
     }
 
     if (n->pmr.dev) {
         if (host_memory_backend_is_mapped(n->pmr.dev)) {
             error_setg(errp, "can't use already busy memdev: %s",
                        object_get_canonical_path_component(OBJECT(n->pmr.dev)));
-            return;
+            return false;
         }
 
         if (!is_power_of_2(n->pmr.dev->size)) {
             error_setg(errp, "pmr backend size needs to be power of 2 in size");
-            return;
+            return false;
         }
 
         host_memory_backend_set_mapped(n->pmr.dev, true);
@@ -7089,64 +7089,64 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
     if (n->params.zasl > n->params.mdts) {
         error_setg(errp, "zoned.zasl (Zone Append Size Limit) must be less "
                    "than or equal to mdts (Maximum Data Transfer Size)");
-        return;
+        return false;
     }
 
     if (!n->params.vsl) {
         error_setg(errp, "vsl must be non-zero");
-        return;
+        return false;
     }
 
     if (params->sriov_max_vfs) {
         if (!n->subsys) {
             error_setg(errp, "subsystem is required for the use of SR-IOV");
-            return;
+            return false;
         }
 
         if (params->sriov_max_vfs > NVME_MAX_VFS) {
             error_setg(errp, "sriov_max_vfs must be between 0 and %d",
                        NVME_MAX_VFS);
-            return;
+            return false;
         }
 
         if (params->cmb_size_mb) {
             error_setg(errp, "CMB is not supported with SR-IOV");
-            return;
+            return false;
         }
 
         if (n->pmr.dev) {
             error_setg(errp, "PMR is not supported with SR-IOV");
-            return;
+            return false;
         }
 
         if (!params->sriov_vq_flexible || !params->sriov_vi_flexible) {
             error_setg(errp, "both sriov_vq_flexible and sriov_vi_flexible"
                        " must be set for the use of SR-IOV");
-            return;
+            return false;
         }
 
         if (params->sriov_vq_flexible < params->sriov_max_vfs * 2) {
             error_setg(errp, "sriov_vq_flexible must be greater than or equal"
                        " to %d (sriov_max_vfs * 2)", params->sriov_max_vfs * 2);
-            return;
+            return false;
         }
 
         if (params->max_ioqpairs < params->sriov_vq_flexible + 2) {
             error_setg(errp, "(max_ioqpairs - sriov_vq_flexible) must be"
                        " greater than or equal to 2");
-            return;
+            return false;
         }
 
         if (params->sriov_vi_flexible < params->sriov_max_vfs) {
             error_setg(errp, "sriov_vi_flexible must be greater than or equal"
                        " to %d (sriov_max_vfs)", params->sriov_max_vfs);
-            return;
+            return false;
         }
 
         if (params->msix_qsize < params->sriov_vi_flexible + 1) {
             error_setg(errp, "(msix_qsize - sriov_vi_flexible) must be"
                        " greater than or equal to 1");
-            return;
+            return false;
         }
 
         if (params->sriov_max_vi_per_vf &&
@@ -7154,7 +7154,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
             error_setg(errp, "sriov_max_vi_per_vf must meet:"
                        " (sriov_max_vi_per_vf - 1) %% %d == 0 and"
                        " sriov_max_vi_per_vf >= 1", NVME_VF_RES_GRANULARITY);
-            return;
+            return false;
         }
 
         if (params->sriov_max_vq_per_vf &&
@@ -7163,9 +7163,11 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
             error_setg(errp, "sriov_max_vq_per_vf must meet:"
                        " (sriov_max_vq_per_vf - 1) %% %d == 0 and"
                        " sriov_max_vq_per_vf >= 2", NVME_VF_RES_GRANULARITY);
-            return;
+            return false;
         }
     }
+
+    return true;
 }
 
 static void nvme_init_state(NvmeCtrl *n)
@@ -7564,7 +7566,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeNamespace *ns;
-    Error *local_err = NULL;
     NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
 
     if (pci_is_vf(pci_dev)) {
@@ -7576,9 +7577,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         n->subsys = pn->subsys;
     }
 
-    nvme_check_constraints(n, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!nvme_check_params(n, errp)) {
         return;
     }
 
@@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
               &pci_dev->qdev, n->parent_obj.qdev.id);
 
     if (nvme_init_subsys(n, errp)) {
-        error_propagate(errp, local_err);
         return;
     }
     nvme_init_state(n);
-- 
2.38.1

Re: [PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen <its@irrelevant.dk> writes:

> From: Klaus Jensen <k.jensen@samsung.com>
>
> Remove an unnecessary local Error value in nvme_realize(). In the
> process, change nvme_check_constraints() into returning a bool.
>
> Finally, removing the local Error value also fixes a bug where an error
> returned from nvme_init_subsys() would be lost.

Would it be lost?  It's the hunk below:

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index ac3885ce5079..a5c0a5fa6ce2 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c

[...]

> @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
   static void nvme_realize(PCIDevice *pci_dev, Error **errp)
   {
       NvmeCtrl *n = NVME(pci_dev);
       NvmeNamespace *ns;
       Error *local_err = NULL;

@local_err is null.

       NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));

       if (pci_is_vf(pci_dev)) {
           /*
            * VFs derive settings from the parent. PF's lifespan exceeds
            * that of VF's, so it's safe to share params.serial.
            */
           memcpy(&n->params, &pn->params, sizeof(NvmeParams));
           n->subsys = pn->subsys;
       }

       nvme_check_constraints(n, &local_err);
       if (local_err) {
           error_propagate(errp, local_err);
           return;
       }

@local_err still is null.

       qbus_init(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
>                &pci_dev->qdev, n->parent_obj.qdev.id);
>  
>      if (nvme_init_subsys(n, errp)) {
> -        error_propagate(errp, local_err);

Since @local_err is null, this error_propagate() does nothing.  The
error from nvme_init_subsys() remains in @errp.

>          return;
>      }
>      nvme_init_state(n);

Re: [PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On Nov 11 07:36, Markus Armbruster wrote:
> Klaus Jensen <its@irrelevant.dk> writes:
> 
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Remove an unnecessary local Error value in nvme_realize(). In the
> > process, change nvme_check_constraints() into returning a bool.
> >
> > Finally, removing the local Error value also fixes a bug where an error
> > returned from nvme_init_subsys() would be lost.
> 
> Would it be lost?  It's the hunk below:
> 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
> >  1 file changed, 23 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index ac3885ce5079..a5c0a5fa6ce2 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> 
> [...]
> 
> > @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>    {
>        NvmeCtrl *n = NVME(pci_dev);
>        NvmeNamespace *ns;
>        Error *local_err = NULL;
> 
> @local_err is null.
> 
>        NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
> 
>        if (pci_is_vf(pci_dev)) {
>            /*
>             * VFs derive settings from the parent. PF's lifespan exceeds
>             * that of VF's, so it's safe to share params.serial.
>             */
>            memcpy(&n->params, &pn->params, sizeof(NvmeParams));
>            n->subsys = pn->subsys;
>        }
> 
>        nvme_check_constraints(n, &local_err);
>        if (local_err) {
>            error_propagate(errp, local_err);
>            return;
>        }
> 
> @local_err still is null.
> 
>        qbus_init(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
> >                &pci_dev->qdev, n->parent_obj.qdev.id);
> >  
> >      if (nvme_init_subsys(n, errp)) {
> > -        error_propagate(errp, local_err);
> 
> Since @local_err is null, this error_propagate() does nothing.  The
> error from nvme_init_subsys() remains in @errp.
> 

Oh, right. Thanks.

I misread the function documentation, getting the impression that it
would overwrite dst_errp regardless of the value of local_err.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen <its@irrelevant.dk> writes:

> On Nov 11 07:36, Markus Armbruster wrote:
>> Klaus Jensen <its@irrelevant.dk> writes:
>> 
>> > From: Klaus Jensen <k.jensen@samsung.com>
>> >
>> > Remove an unnecessary local Error value in nvme_realize(). In the
>> > process, change nvme_check_constraints() into returning a bool.
>> >
>> > Finally, removing the local Error value also fixes a bug where an error
>> > returned from nvme_init_subsys() would be lost.
>> 
>> Would it be lost?  It's the hunk below:
>> 
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> > ---
>> >  hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
>> >  1 file changed, 23 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > index ac3885ce5079..a5c0a5fa6ce2 100644
>> > --- a/hw/nvme/ctrl.c
>> > +++ b/hw/nvme/ctrl.c
>> 
>> [...]
>> 
>> > @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>    {
>>        NvmeCtrl *n = NVME(pci_dev);
>>        NvmeNamespace *ns;
>>        Error *local_err = NULL;
>> 
>> @local_err is null.
>> 
>>        NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
>> 
>>        if (pci_is_vf(pci_dev)) {
>>            /*
>>             * VFs derive settings from the parent. PF's lifespan exceeds
>>             * that of VF's, so it's safe to share params.serial.
>>             */
>>            memcpy(&n->params, &pn->params, sizeof(NvmeParams));
>>            n->subsys = pn->subsys;
>>        }
>> 
>>        nvme_check_constraints(n, &local_err);
>>        if (local_err) {
>>            error_propagate(errp, local_err);
>>            return;
>>        }
>> 
>> @local_err still is null.
>> 
>>        qbus_init(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
>> >                &pci_dev->qdev, n->parent_obj.qdev.id);
>> >  
>> >      if (nvme_init_subsys(n, errp)) {
>> > -        error_propagate(errp, local_err);
>> 
>> Since @local_err is null, this error_propagate() does nothing.  The
>> error from nvme_init_subsys() remains in @errp.
>> 
>
> Oh, right. Thanks.
>
> I misread the function documentation, getting the impression that it
> would overwrite dst_errp regardless of the value of local_err.

Happens :)

If you have suggestions on improving the doc, shoot.

This commit's message could perhaps be adjusted like

    hw/nvme: Clean up confused use of errp/local_err

    Remove an unnecessary local Error value in nvme_realize(). In the
    process, change nvme_check_constraints() to return a bool.

What do you think?

Re: [PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]

On Nov 11 07:55, Markus Armbruster wrote:
> Klaus Jensen <its@irrelevant.dk> writes:
> 
> > On Nov 11 07:36, Markus Armbruster wrote:
> >> Klaus Jensen <its@irrelevant.dk> writes:
> >> 
> >> > From: Klaus Jensen <k.jensen@samsung.com>
> >> >
> >> > Remove an unnecessary local Error value in nvme_realize(). In the
> >> > process, change nvme_check_constraints() into returning a bool.
> >> >
> >> > Finally, removing the local Error value also fixes a bug where an error
> >> > returned from nvme_init_subsys() would be lost.
> >> 
> >> Would it be lost?  It's the hunk below:
> >> 
> >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >> > ---
> >> >  hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
> >> >  1 file changed, 23 insertions(+), 25 deletions(-)
> >> >
> >> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> >> > index ac3885ce5079..a5c0a5fa6ce2 100644
> >> > --- a/hw/nvme/ctrl.c
> >> > +++ b/hw/nvme/ctrl.c
> >> 
> >> [...]
> >> 
> >> > @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >>    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >>    {
> >>        NvmeCtrl *n = NVME(pci_dev);
> >>        NvmeNamespace *ns;
> >>        Error *local_err = NULL;
> >> 
> >> @local_err is null.
> >> 
> >>        NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
> >> 
> >>        if (pci_is_vf(pci_dev)) {
> >>            /*
> >>             * VFs derive settings from the parent. PF's lifespan exceeds
> >>             * that of VF's, so it's safe to share params.serial.
> >>             */
> >>            memcpy(&n->params, &pn->params, sizeof(NvmeParams));
> >>            n->subsys = pn->subsys;
> >>        }
> >> 
> >>        nvme_check_constraints(n, &local_err);
> >>        if (local_err) {
> >>            error_propagate(errp, local_err);
> >>            return;
> >>        }
> >> 
> >> @local_err still is null.
> >> 
> >>        qbus_init(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
> >> >                &pci_dev->qdev, n->parent_obj.qdev.id);
> >> >  
> >> >      if (nvme_init_subsys(n, errp)) {
> >> > -        error_propagate(errp, local_err);
> >> 
> >> Since @local_err is null, this error_propagate() does nothing.  The
> >> error from nvme_init_subsys() remains in @errp.
> >> 
> >
> > Oh, right. Thanks.
> >
> > I misread the function documentation, getting the impression that it
> > would overwrite dst_errp regardless of the value of local_err.
> 
> Happens :)
> 
> If you have suggestions on improving the doc, shoot.
> 
> This commit's message could perhaps be adjusted like
> 
>     hw/nvme: Clean up confused use of errp/local_err
> 
>     Remove an unnecessary local Error value in nvme_realize(). In the
>     process, change nvme_check_constraints() to return a bool.
> 
> What do you think?
> 

Sounds good ;)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] hw/nvme: fix incorrect use of errp/local_err

[Markus Armbruster]

Klaus Jensen <its@irrelevant.dk> writes:

> On Nov 11 07:55, Markus Armbruster wrote:
>> Klaus Jensen <its@irrelevant.dk> writes:
>> 
>> > On Nov 11 07:36, Markus Armbruster wrote:
>> >> Klaus Jensen <its@irrelevant.dk> writes:
>> >> 
>> >> > From: Klaus Jensen <k.jensen@samsung.com>
>> >> >
>> >> > Remove an unnecessary local Error value in nvme_realize(). In the
>> >> > process, change nvme_check_constraints() into returning a bool.
>> >> >
>> >> > Finally, removing the local Error value also fixes a bug where an error
>> >> > returned from nvme_init_subsys() would be lost.
>> >> 
>> >> Would it be lost?  It's the hunk below:
>> >> 
>> >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> >> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> >> > ---
>> >> >  hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
>> >> >  1 file changed, 23 insertions(+), 25 deletions(-)
>> >> >
>> >> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> >> > index ac3885ce5079..a5c0a5fa6ce2 100644
>> >> > --- a/hw/nvme/ctrl.c
>> >> > +++ b/hw/nvme/ctrl.c
>> >> 
>> >> [...]
>> >> 
>> >> > @@ -7586,7 +7585,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>> >>    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>> >>    {
>> >>        NvmeCtrl *n = NVME(pci_dev);
>> >>        NvmeNamespace *ns;
>> >>        Error *local_err = NULL;
>> >> 
>> >> @local_err is null.
>> >> 
>> >>        NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
>> >> 
>> >>        if (pci_is_vf(pci_dev)) {
>> >>            /*
>> >>             * VFs derive settings from the parent. PF's lifespan exceeds
>> >>             * that of VF's, so it's safe to share params.serial.
>> >>             */
>> >>            memcpy(&n->params, &pn->params, sizeof(NvmeParams));
>> >>            n->subsys = pn->subsys;
>> >>        }
>> >> 
>> >>        nvme_check_constraints(n, &local_err);
>> >>        if (local_err) {
>> >>            error_propagate(errp, local_err);
>> >>            return;
>> >>        }
>> >> 
>> >> @local_err still is null.
>> >> 
>> >>        qbus_init(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
>> >> >                &pci_dev->qdev, n->parent_obj.qdev.id);
>> >> >  
>> >> >      if (nvme_init_subsys(n, errp)) {
>> >> > -        error_propagate(errp, local_err);
>> >> 
>> >> Since @local_err is null, this error_propagate() does nothing.  The
>> >> error from nvme_init_subsys() remains in @errp.
>> >> 
>> >
>> > Oh, right. Thanks.
>> >
>> > I misread the function documentation, getting the impression that it
>> > would overwrite dst_errp regardless of the value of local_err.
>> 
>> Happens :)
>> 
>> If you have suggestions on improving the doc, shoot.
>> 
>> This commit's message could perhaps be adjusted like
>> 
>>     hw/nvme: Clean up confused use of errp/local_err
>> 
>>     Remove an unnecessary local Error value in nvme_realize(). In the
>>     process, change nvme_check_constraints() to return a bool.
>> 
>> What do you think?
>
> Sounds good ;)

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Klaus Jensen]

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

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a5c0a5fa6ce2..7d855523bb93 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7343,15 +7343,14 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
     return 0;
 }
 
-static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
+static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
+    ERRP_GUARD();
     uint8_t *pci_conf = pci_dev->config;
     uint64_t bar_size;
     unsigned msix_table_offset, msix_pba_offset;
     int ret;
 
-    Error *err = NULL;
-
     pci_conf[PCI_INTERRUPT_PIN] = 1;
     pci_config_set_prog_interface(pci_conf, 0x2);
 
@@ -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);
+        *errp = NULL;
+    } else if (ret < 0) {
+        return false;
     }
 
     nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
@@ -7412,7 +7409,7 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
         nvme_init_sriov(n, pci_dev, 0x120);
     }
 
-    return 0;
+    return true;
 }
 
 static void nvme_init_subnqn(NvmeCtrl *n)
@@ -7588,7 +7585,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
     nvme_init_state(n);
-    if (nvme_init_pci(n, pci_dev, errp)) {
+    if (!nvme_init_pci(n, pci_dev, errp)) {
         return;
     }
     nvme_init_ctrl(n, pci_dev);
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

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?

> +        *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()?

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

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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>