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.1Klaus 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.
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).
© 2016 - 2026 Red Hat, Inc.