diff mbox series

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

Message ID 20221110220805.26816-2-its@irrelevant.dk
State New
Headers show
Series hw/nvme: errp fixes | expand

Commit Message

Klaus Jensen Nov. 10, 2022, 10:08 p.m. UTC
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(-)

Comments

Markus Armbruster Nov. 11, 2022, 6:36 a.m. UTC | #1
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);
Klaus Jensen Nov. 11, 2022, 6:41 a.m. UTC | #2
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.
Markus Armbruster Nov. 11, 2022, 6:55 a.m. UTC | #3
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?
Klaus Jensen Nov. 11, 2022, 7:09 a.m. UTC | #4
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 ;)
Markus Armbruster Nov. 11, 2022, 11:17 a.m. UTC | #5
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>
diff mbox series

Patch

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