Re: [PULL 26/28] block/nfs: add support for libnfs v6

[Kevin Wolf <kwolf@redhat.com>]

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