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

[PATCH 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 | 60 ++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

-- 
2.38.1

[PATCH 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 an int and fix incorrect use of
errp/local_err.

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..4cc6ae753295 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 int 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 -1;
     }
 
     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 -1;
     }
 
     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 -1;
     }
 
     if (!params->serial) {
         error_setg(errp, "serial property not set");
-        return;
+        return -1;
     }
 
     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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
     }
 
     if (!n->params.vsl) {
         error_setg(errp, "vsl must be non-zero");
-        return;
+        return -1;
     }
 
     if (params->sriov_max_vfs) {
         if (!n->subsys) {
             error_setg(errp, "subsystem is required for the use of SR-IOV");
-            return;
+            return -1;
         }
 
         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 -1;
         }
 
         if (params->cmb_size_mb) {
             error_setg(errp, "CMB is not supported with SR-IOV");
-            return;
+            return -1;
         }
 
         if (n->pmr.dev) {
             error_setg(errp, "PMR is not supported with SR-IOV");
-            return;
+            return -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
 
         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 -1;
         }
     }
+
+    return 0;
 }
 
 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 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 an int and fix incorrect use of
> errp/local_err.
>
> 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..4cc6ae753295 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 int nvme_check_params(NvmeCtrl *n, Error **errp)

I prefer bool true on success, false on failure.  I use int only when it
lets me return additional information, such as a non-negative value on
success, or a negative error code on failure.  nvme_init_pci() is an
example of the latter (although its caller doesn't care).

Local consistency with nvme_init_subsys() is desirable.  You could
convert it to bool, along with nvme_init_pci().  Or you keep all three
int.  Up to you.

>  {
>      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 -1;
>      }
>  
>      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 -1;
>      }
>  
>      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 -1;
>      }
>  
>      if (!params->serial) {
>          error_setg(errp, "serial property not set");
> -        return;
> +        return -1;
>      }
>  
>      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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>      }
>  
>      if (!n->params.vsl) {
>          error_setg(errp, "vsl must be non-zero");
> -        return;
> +        return -1;
>      }
>  
>      if (params->sriov_max_vfs) {
>          if (!n->subsys) {
>              error_setg(errp, "subsystem is required for the use of SR-IOV");
> -            return;
> +            return -1;
>          }
>  
>          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 -1;
>          }
>  
>          if (params->cmb_size_mb) {
>              error_setg(errp, "CMB is not supported with SR-IOV");
> -            return;
> +            return -1;
>          }
>  
>          if (n->pmr.dev) {
>              error_setg(errp, "PMR is not supported with SR-IOV");
> -            return;
> +            return -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>  
>          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 -1;
>          }
>      }
> +
> +    return 0;
>  }
>  
>  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)) {

If you stick to int, then please use

       if (nvme_check_params(n, errp) < 0) {

Here's why.

A bool-valued function that returns false on error we check like

       if (!foo()) {

A pointer-valued function that returns null on error we also check like

       if (!foo()) {

In both cases, convention makes it obvious we're testing for failure.

If you check an int-valued function that returns negative on error like

       if (foo() < 0) {

it's again obvious.

However, if you exploit the fact that it returns zero on success in the
check like

       if (foo()) {

then convention is of no help to readers.  They need to look up what
foo() returns to see whether this is checking for success or for
failure.

Makes sense?

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

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

[Klaus Jensen]

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

On Nov  9 13:29, Markus Armbruster wrote:
> Klaus Jensen <its@irrelevant.dk> writes:
> 
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Make nvme_check_constraints() return an int and fix incorrect use of
> > errp/local_err.
> >
> > 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..4cc6ae753295 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 int nvme_check_params(NvmeCtrl *n, Error **errp)
> 
> I prefer bool true on success, false on failure.  I use int only when it
> lets me return additional information, such as a non-negative value on
> success, or a negative error code on failure.  nvme_init_pci() is an
> example of the latter (although its caller doesn't care).
> 
> Local consistency with nvme_init_subsys() is desirable.  You could
> convert it to bool, along with nvme_init_pci().  Or you keep all three
> int.  Up to you.
> 
> >  {
> >      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 -1;
> >      }
> >  
> >      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 -1;
> >      }
> >  
> >      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 -1;
> >      }
> >  
> >      if (!params->serial) {
> >          error_setg(errp, "serial property not set");
> > -        return;
> > +        return -1;
> >      }
> >  
> >      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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >      }
> >  
> >      if (!n->params.vsl) {
> >          error_setg(errp, "vsl must be non-zero");
> > -        return;
> > +        return -1;
> >      }
> >  
> >      if (params->sriov_max_vfs) {
> >          if (!n->subsys) {
> >              error_setg(errp, "subsystem is required for the use of SR-IOV");
> > -            return;
> > +            return -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          if (params->cmb_size_mb) {
> >              error_setg(errp, "CMB is not supported with SR-IOV");
> > -            return;
> > +            return -1;
> >          }
> >  
> >          if (n->pmr.dev) {
> >              error_setg(errp, "PMR is not supported with SR-IOV");
> > -            return;
> > +            return -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >  
> >          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 -1;
> >          }
> >      }
> > +
> > +    return 0;
> >  }
> >  
> >  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)) {
> 
> If you stick to int, then please use
> 
>        if (nvme_check_params(n, errp) < 0) {
> 
> Here's why.
> 
> A bool-valued function that returns false on error we check like
> 
>        if (!foo()) {
> 
> A pointer-valued function that returns null on error we also check like
> 
>        if (!foo()) {
> 
> In both cases, convention makes it obvious we're testing for failure.
> 
> If you check an int-valued function that returns negative on error like
> 
>        if (foo() < 0) {
> 
> it's again obvious.
> 
> However, if you exploit the fact that it returns zero on success in the
> check like
> 
>        if (foo()) {
> 
> then convention is of no help to readers.  They need to look up what
> foo() returns to see whether this is checking for success or for
> failure.
> 
> Makes sense?
> 

Makes total sense. hw/nvme has traditionally used int return values, but
I definitely agree with your argument.

I'll fix that up (and probably do this across all of hw/nvme for
consistency).

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

Re: [PATCH 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 an int and fix incorrect use of
> errp/local_err.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

One more question: what exactly do you mean by "incorrect use of
errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
merely in the sense of "doesn't use the Error API the way it wants to be
used"?

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

[Klaus Jensen]

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

On Nov  9 13:36, Markus Armbruster wrote:
> Klaus Jensen <its@irrelevant.dk> writes:
> 
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Make nvme_check_constraints() return an int and fix incorrect use of
> > errp/local_err.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> One more question: what exactly do you mean by "incorrect use of
> errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
> merely in the sense of "doesn't use the Error API the way it wants to be
> used"?
> 

It's the last hunk of the patch:

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

It propagates local_err (and it's NULL here).

And the bug is a consequence of the error-prone use of an unneeded local
error value.

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

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

[Markus Armbruster]

Klaus Jensen <its@irrelevant.dk> writes:

> On Nov  9 13:36, Markus Armbruster wrote:
>> Klaus Jensen <its@irrelevant.dk> writes:
>> 
>> > From: Klaus Jensen <k.jensen@samsung.com>
>> >
>> > Make nvme_check_constraints() return an int and fix incorrect use of
>> > errp/local_err.
>> >
>> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> 
>> One more question: what exactly do you mean by "incorrect use of
>> errp/local_err"?  Is it incorrect in the sense of "behaves badly", or
>> merely in the sense of "doesn't use the Error API the way it wants to be
>> used"?
>> 
>
> It's the last hunk of the patch:
>
> @@ -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;
>      }
>
> It propagates local_err (and it's NULL here).

Now I see, thanks!

Harmless, because error_propagate(errp, NULL) does nothing.  Worth
cleaning up all the same.

> And the bug is a consequence of the error-prone use of an unneeded local
> error value.

Yes.  Eliminating unnecessary error_propagate() tends to result in more
concise and clearer code.

[PATCH 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().

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 4cc6ae753295..38eb5ec54f9d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
 
 static int 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,13 +7388,13 @@ 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);
+                    &n->bar0, 0, msix_pba_offset, 0, errp);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
-            warn_report_err(err);
+            warn_report_err(*errp);
+            *errp = NULL;
         } else {
-            error_propagate(errp, err);
-            return ret;
+            return -1;
         }
     }
 
-- 
2.38.1

Re: [PATCH 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().
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 4cc6ae753295..38eb5ec54f9d 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
>  
>  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>  {
> +    ERRP_GUARD();
> +

Drop the blank line, please.

>      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,13 +7388,13 @@ 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);
> +                    &n->bar0, 0, msix_pba_offset, 0, errp);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
> -            warn_report_err(err);
> +            warn_report_err(*errp);
> +            *errp = NULL;
>          } else {
> -            error_propagate(errp, err);
> -            return ret;
> +            return -1;
>          }
>      }

Suggest to reduce nesting:

       if (ret == -ENOTSUP) {
           warn_report_err(*errp);
           *errp = NULL;
       } else if (ret < 0) {
           return -1;
       }

Entirely up to you.

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

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

[Klaus Jensen]

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

On Nov  9 13:33, 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().
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 4cc6ae753295..38eb5ec54f9d 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -7345,13 +7345,13 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
> >  
> >  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
> >  {
> > +    ERRP_GUARD();
> > +
> 
> Drop the blank line, please.
> 

Roger.

> >      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,13 +7388,13 @@ 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);
> > +                    &n->bar0, 0, msix_pba_offset, 0, errp);
> >      if (ret < 0) {
> >          if (ret == -ENOTSUP) {
> > -            warn_report_err(err);
> > +            warn_report_err(*errp);
> > +            *errp = NULL;
> >          } else {
> > -            error_propagate(errp, err);
> > -            return ret;
> > +            return -1;
> >          }
> >      }
> 
> Suggest to reduce nesting:
> 
>        if (ret == -ENOTSUP) {
>            warn_report_err(*errp);
>            *errp = NULL;
>        } else if (ret < 0) {
>            return -1;
>        }
> 
> Entirely up to you.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Makes sense, thanks!

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