| Message ID | 20210824141142.1165291-8-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Rework error reporting | expand |
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 > >
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. > >
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. */
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(-)