Re: [PATCH 7/9] util/vfio-helpers: Have qemu_vfio_dma_map() propagate Error

[Klaus Jensen]

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.

signature.asc
Description: PGP signature