| Message ID | 20221110220805.26816-2-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nvme: errp fixes | expand |
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);
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.
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?
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 ;)
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 --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);