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

[Kevin Wolf <kwolf@redhat.com>]

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