Now that all qemu_vfio_dma_map() callers provide an Error* argument,
fill it with relevant / more descriptive message. Reduce 'ret'
(returned value) scope by returning errno directly to simplify
(removing the goto / 'out' label).
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
block/nvme.c | 1 +
util/vfio-helpers.c | 31 ++++++++++++++-----------------
2 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 663e5d918fa..80546b0babd 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
false, &prp_list_iova, errp);
if (r) {
+ error_prepend(errp, "Cannot map buffer for DMA: ");
goto fail;
}
q->free_req_head = -1;
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 3e1a49bea15..f4c16e1dae5 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
bool temporary, uint64_t *iova, Error **errp)
{
- int ret = 0;
int index;
IOVAMapping *mapping;
uint64_t iova0;
@@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
if (mapping) {
iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
} else {
+ int ret;
+
if (s->high_water_mark - s->low_water_mark + 1 < size) {
- ret = -ENOMEM;
- goto out;
+ error_setg(errp, "iova exhausted (water mark reached)");
+ return -ENOMEM;
}
if (!temporary) {
- if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
- ret = -ENOMEM;
- goto out;
+ if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
+ error_setg(errp, "iova range not found");
+ return -ENOMEM;
}
mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
assert(qemu_vfio_verify_mappings(s));
ret = qemu_vfio_do_mapping(s, host, size, iova0);
- if (ret) {
+ if (ret < 0) {
qemu_vfio_undo_mapping(s, mapping, NULL);
- goto out;
+ return ret;
}
qemu_vfio_dump_mappings(s);
} else {
if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
- ret = -ENOMEM;
- goto out;
+ error_setg(errp, "iova range not found");
+ return -ENOMEM;
}
ret = qemu_vfio_do_mapping(s, host, size, iova0);
- if (ret) {
- goto out;
+ if (ret < 0) {
+ return ret;
}
}
}
@@ -775,11 +776,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
if (iova) {
*iova = iova0;
}
-out:
- if (ret) {
- error_setg_errno(errp, -ret, "Cannot map buffer for DMA");
- }
- return ret;
+ return 0;
}
/* Reset the high watermark and free all "temporary" mappings. */
--
2.31.1
On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
> fill it with relevant / more descriptive message. Reduce 'ret'
> (returned value) scope by returning errno directly to simplify
> (removing the goto / 'out' label).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 1 +
> util/vfio-helpers.c | 31 ++++++++++++++-----------------
> 2 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 663e5d918fa..80546b0babd 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
> r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
> false, &prp_list_iova, errp);
> if (r) {
> + error_prepend(errp, "Cannot map buffer for DMA: ");
Ah. Here is the missing error message that I noticed in patch 2 ;)
> goto fail;
> }
> q->free_req_head = -1;
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 3e1a49bea15..f4c16e1dae5 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
> int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> bool temporary, uint64_t *iova, Error **errp)
> {
> - int ret = 0;
> int index;
> IOVAMapping *mapping;
> uint64_t iova0;
> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> if (mapping) {
> iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
> } else {
> + int ret;
> +
> if (s->high_water_mark - s->low_water_mark + 1 < size) {
> - ret = -ENOMEM;
> - goto out;
> + error_setg(errp, "iova exhausted (water mark reached)");
> + return -ENOMEM;
> }
> if (!temporary) {
> - if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
> - ret = -ENOMEM;
> - goto out;
> + if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
> + error_setg(errp, "iova range not found");
> + return -ENOMEM;
Just curious.
Previously this did error_setg_errno in out. Why don't we want to do that here?
> }
>
> mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0);
> assert(qemu_vfio_verify_mappings(s));
> ret = qemu_vfio_do_mapping(s, host, size, iova0);
> - if (ret) {
> + if (ret < 0) {
> qemu_vfio_undo_mapping(s, mapping, NULL);
> - goto out;
> + return ret;
> }
> qemu_vfio_dump_mappings(s);
> } else {
> if (qemu_vfio_find_temp_iova(s, size, &iova0)) {
> - ret = -ENOMEM;
> - goto out;
> + error_setg(errp, "iova range not found");
> + return -ENOMEM;
> }
> ret = qemu_vfio_do_mapping(s, host, size, iova0);
> - if (ret) {
> - goto out;
> + if (ret < 0) {
> + return ret;
> }
> }
> }
> @@ -775,11 +776,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> if (iova) {
> *iova = iova0;
> }
> -out:
> - if (ret) {
> - error_setg_errno(errp, -ret, "Cannot map buffer for DMA");
> - }
> - return ret;
> + return 0;
> }
>
> /* Reset the high watermark and free all "temporary" mappings. */
> --
> 2.31.1
>
>
--
One of us - No more doubt, silence or taboo about mental illness.
On 8/25/21 1:34 PM, Klaus Jensen wrote:
> On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
>> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
>> fill it with relevant / more descriptive message. Reduce 'ret'
>> (returned value) scope by returning errno directly to simplify
>> (removing the goto / 'out' label).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> block/nvme.c | 1 +
>> util/vfio-helpers.c | 31 ++++++++++++++-----------------
>> 2 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 663e5d918fa..80546b0babd 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>> r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
>> false, &prp_list_iova, errp);
>> if (r) {
>> + error_prepend(errp, "Cannot map buffer for DMA: ");
>
> Ah. Here is the missing error message that I noticed in patch 2 ;)
So I should use error_setg() in patch 2 and replace it with
error_prepend() here? I'm trying to address Eric's comments
from:
https://lore.kernel.org/qemu-devel/2cbd5471-4611-ae6b-d79f-db6ff19db5bf@redhat.com/
https://lore.kernel.org/qemu-devel/bd1cc1c9-3553-d252-3ca9-a23bc1035170@redhat.com/
>
>> goto fail;
>> }
>> q->free_req_head = -1;
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 3e1a49bea15..f4c16e1dae5 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
>> int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>> bool temporary, uint64_t *iova, Error **errp)
>> {
>> - int ret = 0;
>> int index;
>> IOVAMapping *mapping;
>> uint64_t iova0;
>> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>> if (mapping) {
>> iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
>> } else {
>> + int ret;
>> +
>> if (s->high_water_mark - s->low_water_mark + 1 < size) {
>> - ret = -ENOMEM;
>> - goto out;
>> + error_setg(errp, "iova exhausted (water mark reached)");
>> + return -ENOMEM;
>> }
>> if (!temporary) {
>> - if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
>> - ret = -ENOMEM;
>> - goto out;
>> + if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
>> + error_setg(errp, "iova range not found");
>> + return -ENOMEM;
>
> Just curious.
>
> Previously this did error_setg_errno in out. Why don't we want to do that here?
I didn't thought about it but indeed you are right.
Thanks for the review!
Phil.
On Aug 25 15:08, Philippe Mathieu-Daudé wrote:
> On 8/25/21 1:34 PM, Klaus Jensen wrote:
> > On Aug 24 16:11, Philippe Mathieu-Daudé wrote:
> >> Now that all qemu_vfio_dma_map() callers provide an Error* argument,
> >> fill it with relevant / more descriptive message. Reduce 'ret'
> >> (returned value) scope by returning errno directly to simplify
> >> (removing the goto / 'out' label).
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> block/nvme.c | 1 +
> >> util/vfio-helpers.c | 31 ++++++++++++++-----------------
> >> 2 files changed, 15 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 663e5d918fa..80546b0babd 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -240,6 +240,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
> >> r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
> >> false, &prp_list_iova, errp);
> >> if (r) {
> >> + error_prepend(errp, "Cannot map buffer for DMA: ");
> >
> > Ah. Here is the missing error message that I noticed in patch 2 ;)
>
> So I should use error_setg() in patch 2 and replace it with
> error_prepend() here? I'm trying to address Eric's comments
> from:
>
> https://lore.kernel.org/qemu-devel/2cbd5471-4611-ae6b-d79f-db6ff19db5bf@redhat.com/
> https://lore.kernel.org/qemu-devel/bd1cc1c9-3553-d252-3ca9-a23bc1035170@redhat.com/
>
It's not like its breaking anything that it is missing from patch 2, so
I'm fine with it not being there. I agree that it feels weird to add it,
then replace it a few patches further down ;)
> >
> >> goto fail;
> >> }
> >> q->free_req_head = -1;
> >> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> >> index 3e1a49bea15..f4c16e1dae5 100644
> >> --- a/util/vfio-helpers.c
> >> +++ b/util/vfio-helpers.c
> >> @@ -729,7 +729,6 @@ qemu_vfio_find_temp_iova(QEMUVFIOState *s, size_t size, uint64_t *iova)
> >> int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> >> bool temporary, uint64_t *iova, Error **errp)
> >> {
> >> - int ret = 0;
> >> int index;
> >> IOVAMapping *mapping;
> >> uint64_t iova0;
> >> @@ -742,32 +741,34 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
> >> if (mapping) {
> >> iova0 = mapping->iova + ((uint8_t *)host - (uint8_t *)mapping->host);
> >> } else {
> >> + int ret;
> >> +
> >> if (s->high_water_mark - s->low_water_mark + 1 < size) {
> >> - ret = -ENOMEM;
> >> - goto out;
> >> + error_setg(errp, "iova exhausted (water mark reached)");
> >> + return -ENOMEM;
> >> }
> >> if (!temporary) {
> >> - if (qemu_vfio_find_fixed_iova(s, size, &iova0)) {
> >> - ret = -ENOMEM;
> >> - goto out;
> >> + if (qemu_vfio_find_fixed_iova(s, size, &iova0) < 0) {
> >> + error_setg(errp, "iova range not found");
> >> + return -ENOMEM;
> >
> > Just curious.
> >
> > Previously this did error_setg_errno in out. Why don't we want to do that here?
>
> I didn't thought about it but indeed you are right.
>
> Thanks for the review!
>
> Phil.
>
>
--
One of us - No more doubt, silence or taboo about mental illness.
© 2016 - 2026 Red Hat, Inc.