| Message ID | 20210824141142.1165291-6-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/nvme: Rework error reporting | expand |
On Aug 24 16:11, Philippe Mathieu-Daudé wrote: > qemu_vfio_add_mapping() returns a pointer to an indexed entry > in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL. > Remove the pointless check. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > util/vfio-helpers.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c > index d956866b4e9..e7909222cfd 100644 > --- a/util/vfio-helpers.c > +++ b/util/vfio-helpers.c > @@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size, > } > > mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0); > - if (!mapping) { > - ret = -ENOMEM; > - goto out; > - } > assert(qemu_vfio_verify_mappings(s)); > ret = qemu_vfio_do_mapping(s, host, size, iova0); > if (ret) { > -- > 2.31.1 > > This looks OK. But maybe it would be prudent to assert that index is within bounds of s->mappings in qemu_vfio_add_mapping? E.g., assert(index >= 0 && index < s->nr_mappings + 1);
On Aug 25 13:53, Klaus Jensen wrote: > On Aug 24 16:11, Philippe Mathieu-Daudé wrote: > > qemu_vfio_add_mapping() returns a pointer to an indexed entry > > in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL. > > Remove the pointless check. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > util/vfio-helpers.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c > > index d956866b4e9..e7909222cfd 100644 > > --- a/util/vfio-helpers.c > > +++ b/util/vfio-helpers.c > > @@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size, > > } > > > > mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0); > > - if (!mapping) { > > - ret = -ENOMEM; > > - goto out; > > - } > > assert(qemu_vfio_verify_mappings(s)); > > ret = qemu_vfio_do_mapping(s, host, size, iova0); > > if (ret) { > > -- > > 2.31.1 > > > > > > This looks OK. > > But maybe it would be prudent to assert that index is within bounds of > s->mappings in qemu_vfio_add_mapping? E.g., > > assert(index >= 0 && index < s->nr_mappings + 1); It's not like `if (!mapping) {` would have cought and out-of-bounds index value anyway, so Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c index d956866b4e9..e7909222cfd 100644 --- a/util/vfio-helpers.c +++ b/util/vfio-helpers.c @@ -751,10 +751,6 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size, } mapping = qemu_vfio_add_mapping(s, host, size, index + 1, iova0); - if (!mapping) { - ret = -ENOMEM; - goto out; - } assert(qemu_vfio_verify_mappings(s)); ret = qemu_vfio_do_mapping(s, host, size, iova0); if (ret) {
qemu_vfio_add_mapping() returns a pointer to an indexed entry in pre-allocated QEMUVFIOState::mappings[], thus can not be NULL. Remove the pointless check. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- util/vfio-helpers.c | 4 ---- 1 file changed, 4 deletions(-)