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

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

[Klaus Jensen]

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

Fix a couple of invalid errp usages.

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 v2 1/2] hw/nvme: fix incorrect use of errp/local_err

[Klaus Jensen]

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

Make nvme_check_constraints() return a bool and fix an invalid error
propagation where the actual error is thrown away in favor of an unused
local Error value.

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 v2 1/2] hw/nvme: fix incorrect use of errp/local_err

[Philippe Mathieu-Daudé]

On 10/11/22 07:23, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Make nvme_check_constraints() return a bool and fix an invalid error
> propagation where the actual error is thrown away in favor of an unused
> local Error value.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
>   1 file changed, 23 insertions(+), 25 deletions(-)

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

Similarly nvme_init_subsys() could return a boolean.

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

> -        error_propagate(errp, local_err);
>           return;
>       }
>       nvme_init_state(n);

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

[Klaus Jensen]

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

On Nov 10 10:59, Philippe Mathieu-Daudé wrote:
> On 10/11/22 07:23, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Make nvme_check_constraints() return a bool and fix an invalid error
> > propagation where the actual error is thrown away in favor of an unused
> > local Error value.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 48 +++++++++++++++++++++++-------------------------
> >   1 file changed, 23 insertions(+), 25 deletions(-)
> 
> > @@ -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)) {
> 
> Similarly nvme_init_subsys() could return a boolean.
> 

Yes, Markus pointed that out as well. A lot of functions would benefit
from similar changes, but I'd rather defer that until after 7.2 ;)

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

Re: [PATCH v2 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>
>
> Make nvme_check_constraints() return a bool and fix an invalid error
> propagation where the actual error is thrown away in favor of an unused
> local Error value.

Is it?  I think you're talking about ...

> 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

[...]

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

... this one.  error_propagate() does nothing here, because @local_err
is still null.

>          return;
>      }
>      nvme_init_state(n);

[PATCH v2 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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
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..e23af4db91ae 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 {
+        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 v2 2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

[Markus Armbruster]

Klaus Jensen <its@irrelevant.dk> writes:

> From: Klaus Jensen <k.jensen@samsung.com>
>
> Replace the local Error variable with errp and ERRP_GUARD() and change
> the return value to bool.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 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..e23af4db91ae 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 {

Uh, shouldn't this be

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

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

[Klaus Jensen]

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

On Nov 10 11:00, Markus Armbruster wrote:
> Klaus Jensen <its@irrelevant.dk> writes:
> 
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Replace the local Error variable with errp and ERRP_GUARD() and change
> > the return value to bool.
> >
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > 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..e23af4db91ae 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 {
> 
> Uh, shouldn't this be
> 
>        } else if (ret < 0) {
> 
> ?
> 

Yikes. Yes.

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