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é <phi...@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-db6ff19db...@redhat.com/
https://lore.kernel.org/qemu-devel/bd1cc1c9-3553-d252-3ca9-a23bc1035...@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.
