Re: [PULL 26/28] block/nfs: add support for libnfs v6
Am 09.04.2026 um 11:48 hat Peter Maydell geschrieben:
> On Fri, 20 Mar 2026 at 09:50, Peter Maydell wrote:
> >
> > I just noticed I forgot to cc Peter Lieven on this email -- sorry
> > about that.
> >
> > On Thu, 12 Mar 2026 at 16:47, Kevin Wolf wrote:
> > >
> > > Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben:
> > > > On Thu, 12 Mar 2026 at 16:13, Kevin Wolf wrote:
> > > > >
> > > > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> > > > > > Maybe we can't get here, but maybe it would be simpler to handle
> > > > > > the "asked to read 0 bytes" case directly without calling into the
> > > > > > nfs library or allocating a 0 byte buffer?
> >
> > > > > We could have an 'if (!bytes) { return 0; }' at the start of the
> > > > > function if we want to.
> >
> > > > Mmm. Let me know if you'd prefer me to mark this issue in
> > > > Coverity as a false positive rather than changing the code.
> > >
> > > I don't really mind either way. If someone posts a patch, I'll apply it
> > > (though not sure if that would be only for 11.1 at this point).
> >
> > So what is the conclusion here -- do we want to change the code,
> > or are we happy with it as it is and want to tell Coverity this
> > is a false positive?
>
> Ping. I would like to know whether we want to make a change here,
> or if I should mark this as a false-positive in Coverity.
Seems nobody cared enough to send a patch, so yes, let's mark it as a
false positive.
Kevin
Re: [PULL 26/28] block/nfs: add support for libnfs v6
On Fri, 20 Mar 2026 at 09:50, Peter Maydell wrote:
>
> I just noticed I forgot to cc Peter Lieven on this email -- sorry
> about that.
>
> On Thu, 12 Mar 2026 at 16:47, Kevin Wolf wrote:
> >
> > Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben:
> > > On Thu, 12 Mar 2026 at 16:13, Kevin Wolf wrote:
> > > >
> > > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> > > > > Maybe we can't get here, but maybe it would be simpler to handle
> > > > > the "asked to read 0 bytes" case directly without calling into the
> > > > > nfs library or allocating a 0 byte buffer?
>
> > > > We could have an 'if (!bytes) { return 0; }' at the start of the
> > > > function if we want to.
>
> > > Mmm. Let me know if you'd prefer me to mark this issue in
> > > Coverity as a false positive rather than changing the code.
> >
> > I don't really mind either way. If someone posts a patch, I'll apply it
> > (though not sure if that would be only for 11.1 at this point).
>
> So what is the conclusion here -- do we want to change the code,
> or are we happy with it as it is and want to tell Coverity this
> is a false positive?
Ping. I would like to know whether we want to make a change here,
or if I should mark this as a false-positive in Coverity.
thanks
-- PMM
Re: [PULL 26/28] block/nfs: add support for libnfs v6
I just noticed I forgot to cc Peter Lieven on this email -- sorry about that. On Thu, 12 Mar 2026 at 16:47, Kevin Wolf wrote: > > Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben: > > On Thu, 12 Mar 2026 at 16:13, Kevin Wolf wrote: > > > > > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben: > > > > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf wrote: > > > > > > > > > > From: Peter Lieven > > > > > > > > > > libnfs v6 added a new api structure for read and write requests. > > > > > > > > > > This effectively also adds zero copy read support for cases where > > > > > the qiov coming from the block layer has only one vector. > > > > > > > > > > The .brdv_refresh_limits implementation is needed because libnfs v6 > > > > > silently dropped support for splitting large read/write request into > > > > > chunks. > > > > > > > > > > Signed-off-by: Ronnie Sahlberg > > > > > Signed-off-by: Peter Lieven > > > > > Message-ID: <[email protected]> > > > > > Reviewed-by: Kevin Wolf > > > > > Signed-off-by: Kevin Wolf > > > > > > > > > > > > Hi; Coverity reports a potential issue with this code > > > > (CID 1645631): > > > > > +if (my_buffer) { > > > > > +if (task.ret > 0) { > > > > > +qemu_iovec_from_buf(iov, 0, buf, task.ret); > > > > > > > > ...and down here we use 'buf', but Coverity thinks it might be NULL > > > > because we only returned -ENOMEM above for a NULL buffer if bytes == 0. > > > > So we might be here with bytes == 0 and buf == NULL. > > > > > > Yes, buf might be NULL, but copying 0 bytes from NULL isn't a problem > > > because you don't actually dereference the pointer then. > > > > > > I think the part that Coverity doesn't understand is probably that > > > task.ret is limited to bytes (i.e. 0 in this case). > > > > Maybe we can't get here, but maybe it would be simpler to handle > > > > the "asked to read 0 bytes" case directly without calling into the > > > > nfs library or allocating a 0 byte buffer? > > > We could have an 'if (!bytes) { return 0; }' at the start of the > > > function if we want to. > > Mmm. Let me know if you'd prefer me to mark this issue in > > Coverity as a false positive rather than changing the code. > > I don't really mind either way. If someone posts a patch, I'll apply it > (though not sure if that would be only for 11.1 at this point). So what is the conclusion here -- do we want to change the code, or are we happy with it as it is and want to tell Coverity this is a false positive? thanks -- PMM
Re: [PULL 26/28] block/nfs: add support for libnfs v6
Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben: > On Thu, 12 Mar 2026 at 16:13, Kevin Wolf wrote: > > > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben: > > > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf wrote: > > > > > > > > From: Peter Lieven > > > > > > > > libnfs v6 added a new api structure for read and write requests. > > > > > > > > This effectively also adds zero copy read support for cases where > > > > the qiov coming from the block layer has only one vector. > > > > > > > > The .brdv_refresh_limits implementation is needed because libnfs v6 > > > > silently dropped support for splitting large read/write request into > > > > chunks. > > > > > > > > Signed-off-by: Ronnie Sahlberg > > > > Signed-off-by: Peter Lieven > > > > Message-ID: <[email protected]> > > > > Reviewed-by: Kevin Wolf > > > > Signed-off-by: Kevin Wolf > > > > > > > > > Hi; Coverity reports a potential issue with this code > > > (CID 1645631): > > > > > > > @@ -266,13 +270,36 @@ static int coroutine_fn > > > > nfs_co_preadv(BlockDriverState *bs, int64_t offset, > > > > { > > > > NFSClient *client = bs->opaque; > > > > NFSRPC task; > > > > +char *buf = NULL; > > > > +bool my_buffer = false; > > > > > > > > nfs_co_init_task(bs, &task); > > > > -task.iov = iov; > > > > + > > > > +#ifdef LIBNFS_API_V2 > > > > +if (iov->niov != 1) { > > > > +buf = g_try_malloc(bytes); > > > > +if (bytes && buf == NULL) { > > > > +return -ENOMEM; > > > > +} > > > > +my_buffer = true; > > > > > > Here we have code that takes the "read zero bytes" case, and > > > still tries to malloc a 0-length buffer (which is of dubious > > > portability). Then it will continue... > > > > g_try_malloc() always returns NULL for allocating 0 bytes, so I don't > > think portability is a problem here. > > Ah, so it does. The glib docs document this for g_malloc() but > don't mention it for g_try_malloc(), so I missed it. > > > > > +if (my_buffer) { > > > > +if (task.ret > 0) { > > > > +qemu_iovec_from_buf(iov, 0, buf, task.ret); > > > > > > ...and down here we use 'buf', but Coverity thinks it might be NULL > > > because we only returned -ENOMEM above for a NULL buffer if bytes == 0. > > > So we might be here with bytes == 0 and buf == NULL. > > > > Yes, buf might be NULL, but copying 0 bytes from NULL isn't a problem > > because you don't actually dereference the pointer then. > > > > I think the part that Coverity doesn't understand is probably that > > task.ret is limited to bytes (i.e. 0 in this case). > > Mmm. Let me know if you'd prefer me to mark this issue in > Coverity as a false positive rather than changing the code. I don't really mind either way. If someone posts a patch, I'll apply it (though not sure if that would be only for 11.1 at this point). Kevin
Re: [PULL 26/28] block/nfs: add support for libnfs v6
On Thu, 12 Mar 2026 at 16:13, Kevin Wolf wrote: > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben: > > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf wrote: > > > > > > From: Peter Lieven > > > > > > libnfs v6 added a new api structure for read and write requests. > > > > > > This effectively also adds zero copy read support for cases where > > > the qiov coming from the block layer has only one vector. > > > > > > The .brdv_refresh_limits implementation is needed because libnfs v6 > > > silently dropped support for splitting large read/write request into > > > chunks. > > > > > > Signed-off-by: Ronnie Sahlberg > > > Signed-off-by: Peter Lieven > > > Message-ID: <[email protected]> > > > Reviewed-by: Kevin Wolf > > > Signed-off-by: Kevin Wolf > > > > > > Hi; Coverity reports a potential issue with this code > > (CID 1645631): > > > > > @@ -266,13 +270,36 @@ static int coroutine_fn > > > nfs_co_preadv(BlockDriverState *bs, int64_t offset, > > > { > > > NFSClient *client = bs->opaque; > > > NFSRPC task; > > > +char *buf = NULL; > > > +bool my_buffer = false; > > > > > > nfs_co_init_task(bs, &task); > > > -task.iov = iov; > > > + > > > +#ifdef LIBNFS_API_V2 > > > +if (iov->niov != 1) { > > > +buf = g_try_malloc(bytes); > > > +if (bytes && buf == NULL) { > > > +return -ENOMEM; > > > +} > > > +my_buffer = true; > > > > Here we have code that takes the "read zero bytes" case, and > > still tries to malloc a 0-length buffer (which is of dubious > > portability). Then it will continue... > > g_try_malloc() always returns NULL for allocating 0 bytes, so I don't > think portability is a problem here. Ah, so it does. The glib docs document this for g_malloc() but don't mention it for g_try_malloc(), so I missed it. > > > +if (my_buffer) { > > > +if (task.ret > 0) { > > > +qemu_iovec_from_buf(iov, 0, buf, task.ret); > > > > ...and down here we use 'buf', but Coverity thinks it might be NULL > > because we only returned -ENOMEM above for a NULL buffer if bytes == 0. > > So we might be here with bytes == 0 and buf == NULL. > > Yes, buf might be NULL, but copying 0 bytes from NULL isn't a problem > because you don't actually dereference the pointer then. > > I think the part that Coverity doesn't understand is probably that > task.ret is limited to bytes (i.e. 0 in this case). Mmm. Let me know if you'd prefer me to mark this issue in Coverity as a false positive rather than changing the code. -- PMM
Re: [PULL 26/28] block/nfs: add support for libnfs v6
Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben: > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf wrote: > > > > From: Peter Lieven > > > > libnfs v6 added a new api structure for read and write requests. > > > > This effectively also adds zero copy read support for cases where > > the qiov coming from the block layer has only one vector. > > > > The .brdv_refresh_limits implementation is needed because libnfs v6 > > silently dropped support for splitting large read/write request into > > chunks. > > > > Signed-off-by: Ronnie Sahlberg > > Signed-off-by: Peter Lieven > > Message-ID: <[email protected]> > > Reviewed-by: Kevin Wolf > > Signed-off-by: Kevin Wolf > > > Hi; Coverity reports a potential issue with this code > (CID 1645631): > > > @@ -266,13 +270,36 @@ static int coroutine_fn > > nfs_co_preadv(BlockDriverState *bs, int64_t offset, > > { > > NFSClient *client = bs->opaque; > > NFSRPC task; > > +char *buf = NULL; > > +bool my_buffer = false; > > > > nfs_co_init_task(bs, &task); > > -task.iov = iov; > > + > > +#ifdef LIBNFS_API_V2 > > +if (iov->niov != 1) { > > +buf = g_try_malloc(bytes); > > +if (bytes && buf == NULL) { > > +return -ENOMEM; > > +} > > +my_buffer = true; > > Here we have code that takes the "read zero bytes" case, and > still tries to malloc a 0-length buffer (which is of dubious > portability). Then it will continue... g_try_malloc() always returns NULL for allocating 0 bytes, so I don't think portability is a problem here. > > +} else { > > +buf = iov->iov[0].iov_base; > > +} > > +#endif > > > > WITH_QEMU_LOCK_GUARD(&client->mutex) { > > +#ifdef LIBNFS_API_V2 > > +if (nfs_pread_async(client->context, client->fh, > > +buf, bytes, offset, > > +nfs_co_generic_cb, &task) != 0) { > > +#else > > +task.iov = iov; > > if (nfs_pread_async(client->context, client->fh, > > offset, bytes, nfs_co_generic_cb, &task) != 0) > > { > > +#endif > > +if (my_buffer) { > > +g_free(buf); > > +} > > return -ENOMEM; > > } > > > > @@ -280,6 +307,13 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState > > *bs, int64_t offset, > > } > > qemu_coroutine_yield(); > > > > +if (my_buffer) { > > +if (task.ret > 0) { > > +qemu_iovec_from_buf(iov, 0, buf, task.ret); > > ...and down here we use 'buf', but Coverity thinks it might be NULL > because we only returned -ENOMEM above for a NULL buffer if bytes == 0. > So we might be here with bytes == 0 and buf == NULL. Yes, buf might be NULL, but copying 0 bytes from NULL isn't a problem because you don't actually dereference the pointer then. I think the part that Coverity doesn't understand is probably that task.ret is limited to bytes (i.e. 0 in this case). > Maybe we can't get here, but maybe it would be simpler to handle > the "asked to read 0 bytes" case directly without calling into the > nfs library or allocating a 0 byte buffer? We could have an 'if (!bytes) { return 0; }' at the start of the function if we want to. Kevin
Re: [PULL 26/28] block/nfs: add support for libnfs v6
On Tue, 10 Mar 2026 at 16:30, Kevin Wolf wrote: > > From: Peter Lieven > > libnfs v6 added a new api structure for read and write requests. > > This effectively also adds zero copy read support for cases where > the qiov coming from the block layer has only one vector. > > The .brdv_refresh_limits implementation is needed because libnfs v6 > silently dropped support for splitting large read/write request into > chunks. > > Signed-off-by: Ronnie Sahlberg > Signed-off-by: Peter Lieven > Message-ID: <[email protected]> > Reviewed-by: Kevin Wolf > Signed-off-by: Kevin Wolf Hi; Coverity reports a potential issue with this code (CID 1645631): > @@ -266,13 +270,36 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState > *bs, int64_t offset, > { > NFSClient *client = bs->opaque; > NFSRPC task; > +char *buf = NULL; > +bool my_buffer = false; > > nfs_co_init_task(bs, &task); > -task.iov = iov; > + > +#ifdef LIBNFS_API_V2 > +if (iov->niov != 1) { > +buf = g_try_malloc(bytes); > +if (bytes && buf == NULL) { > +return -ENOMEM; > +} > +my_buffer = true; Here we have code that takes the "read zero bytes" case, and still tries to malloc a 0-length buffer (which is of dubious portability). Then it will continue... > +} else { > +buf = iov->iov[0].iov_base; > +} > +#endif > > WITH_QEMU_LOCK_GUARD(&client->mutex) { > +#ifdef LIBNFS_API_V2 > +if (nfs_pread_async(client->context, client->fh, > +buf, bytes, offset, > +nfs_co_generic_cb, &task) != 0) { > +#else > +task.iov = iov; > if (nfs_pread_async(client->context, client->fh, > offset, bytes, nfs_co_generic_cb, &task) != 0) { > +#endif > +if (my_buffer) { > +g_free(buf); > +} > return -ENOMEM; > } > > @@ -280,6 +307,13 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState > *bs, int64_t offset, > } > qemu_coroutine_yield(); > > +if (my_buffer) { > +if (task.ret > 0) { > +qemu_iovec_from_buf(iov, 0, buf, task.ret); ...and down here we use 'buf', but Coverity thinks it might be NULL because we only returned -ENOMEM above for a NULL buffer if bytes == 0. So we might be here with bytes == 0 and buf == NULL. Maybe we can't get here, but maybe it would be simpler to handle the "asked to read 0 bytes" case directly without calling into the nfs library or allocating a 0 byte buffer? > +} > +g_free(buf); > +} > + > if (task.ret < 0) { > return task.ret; > } thanks -- PMM
