From: Peter Lieven <pl@dlhnet.de>

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 <ronniesahlberg@gmail.com>
Signed-off-by: Peter Lieven <pl@dlhnet.de>
Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nfs.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 meson.build |  2 +-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index b78f4f86e85..53e267fa755 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -69,7 +69,9 @@ typedef struct NFSClient {
 typedef struct NFSRPC {
     BlockDriverState *bs;
     int ret;
+#ifndef LIBNFS_API_V2
     QEMUIOVector *iov;
+#endif
     struct stat *st;
     Coroutine *co;
     NFSClient *client;
@@ -237,6 +239,7 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
     NFSRPC *task = private_data;
     task->ret = ret;
     assert(!task->st);
+#ifndef LIBNFS_API_V2
     if (task->ret > 0 && task->iov) {
         if (task->ret <= task->iov->size) {
             qemu_iovec_from_buf(task->iov, 0, data, task->ret);
@@ -244,6 +247,7 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
             task->ret = -EIO;
         }
     }
+#endif
     if (task->ret < 0) {
         error_report("NFS Error: %s", nfs_get_error(nfs));
     }
@@ -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;
+    } 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);
+        }
+        g_free(buf);
+    }
+
     if (task.ret < 0) {
         return task.ret;
     }
@@ -315,9 +349,15 @@ static int coroutine_fn nfs_co_pwritev(BlockDriverState *bs, int64_t offset,
     }
 
     WITH_QEMU_LOCK_GUARD(&client->mutex) {
+#ifdef LIBNFS_API_V2
+        if (nfs_pwrite_async(client->context, client->fh,
+                             buf, bytes, offset,
+                             nfs_co_generic_cb, &task) != 0) {
+#else
         if (nfs_pwrite_async(client->context, client->fh,
                              offset, bytes, buf,
                              nfs_co_generic_cb, &task) != 0) {
+#endif
             if (my_buffer) {
                 g_free(buf);
             }
@@ -856,6 +896,13 @@ static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs,
 }
 #endif
 
+static void nfs_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    NFSClient *client = bs->opaque;
+    bs->bl.max_transfer = MIN((uint32_t)nfs_get_readmax(client->context),
+                              (uint32_t)nfs_get_writemax(client->context));
+}
+
 static const char *nfs_strong_runtime_opts[] = {
     "path",
     "user",
@@ -893,6 +940,7 @@ static BlockDriver bdrv_nfs = {
     .bdrv_detach_aio_context        = nfs_detach_aio_context,
     .bdrv_attach_aio_context        = nfs_attach_aio_context,
     .bdrv_refresh_filename          = nfs_refresh_filename,
+    .bdrv_refresh_limits            = nfs_refresh_limits,
     .bdrv_dirname                   = nfs_dirname,
 
     .strong_runtime_opts            = nfs_strong_runtime_opts,
diff --git a/meson.build b/meson.build
index f45885f05a1..bb0d1d993a5 100644
--- a/meson.build
+++ b/meson.build
@@ -1157,7 +1157,7 @@ endif
 
 libnfs = not_found
 if not get_option('libnfs').auto() or have_block
-  libnfs = dependency('libnfs', version: ['>=1.9.3', '<6.0.0'],
+  libnfs = dependency('libnfs', version: '>=1.9.3',
                       required: get_option('libnfs'),
                       method: 'pkg-config')
 endif
-- 
2.53.0

On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <kwolf@redhat.com> wrote:
>
> From: Peter Lieven <pl@dlhnet.de>
>
> 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 <ronniesahlberg@gmail.com>
> Signed-off-by: Peter Lieven <pl@dlhnet.de>
> Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


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

Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > From: Peter Lieven <pl@dlhnet.de>
> >
> > 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 <ronniesahlberg@gmail.com>
> > Signed-off-by: Peter Lieven <pl@dlhnet.de>
> > Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> 
> 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

On Thu, 12 Mar 2026 at 16:13, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > From: Peter Lieven <pl@dlhnet.de>
> > >
> > > 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 <ronniesahlberg@gmail.com>
> > > Signed-off-by: Peter Lieven <pl@dlhnet.de>
> > > Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
> > > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >
> >
> > 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

Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben:
> On Thu, 12 Mar 2026 at 16:13, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> > > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <kwolf@redhat.com> wrote:
> > > >
> > > > From: Peter Lieven <pl@dlhnet.de>
> > > >
> > > > 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 <ronniesahlberg@gmail.com>
> > > > Signed-off-by: Peter Lieven <pl@dlhnet.de>
> > > > Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
> > > > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > >
> > >
> > > 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

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 <kwolf@redhat.com> wrote:
>
> Am 12.03.2026 um 17:19 hat Peter Maydell geschrieben:
> > On Thu, 12 Mar 2026 at 16:13, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 12.03.2026 um 10:41 hat Peter Maydell geschrieben:
> > > > On Tue, 10 Mar 2026 at 16:30, Kevin Wolf <kwolf@redhat.com> wrote:
> > > > >
> > > > > From: Peter Lieven <pl@dlhnet.de>
> > > > >
> > > > > 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 <ronniesahlberg@gmail.com>
> > > > > Signed-off-by: Peter Lieven <pl@dlhnet.de>
> > > > > Message-ID: <20260306142840.72923-1-pl@dlhnet.de>
> > > > > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > >
> > > >
> > > > 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