On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <[email protected]> wrote:
>
> From: Peter Lieven <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Peter Lieven <[email protected]>
> Message-ID: <[email protected]>
> Reviewed-by: Kevin Wolf <[email protected]>
> Signed-off-by: Kevin Wolf <[email protected]>
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
