| Message ID | 20221109105357.30430-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> > > 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);
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).
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"?
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.
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.
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);