On Wed, 20 Jul 2022 at 10:04, Jason Wang <jasow...@redhat.com> wrote: > > From: Eugenio Pérez <epere...@redhat.com> > > To know the device features is needed for CVQ SVQ, so SVQ knows if it > can handle all commands or not. Extract from > vhost_vdpa_get_max_queue_pairs so we can reuse it. > > Signed-off-by: Eugenio Pérez <epere...@redhat.com> > Acked-by: Jason Wang <jasow...@redhat.com> > Reviewed-by: Michael S. Tsirkin <m...@redhat.com> > Signed-off-by: Jason Wang <jasow...@redhat.com>
Hi; this change introduces a resource leak in the new error-exit return path in net_init_vhost_vdpa(). Spotted by Coverity, CID 1490785. > @@ -517,10 +521,11 @@ int net_init_vhost_vdpa(const Netdev *netdev, const > char *name, > NetClientState *peer, Error **errp) > { > const NetdevVhostVDPAOptions *opts; > + uint64_t features; > int vdpa_device_fd; > g_autofree NetClientState **ncs = NULL; > NetClientState *nc; > - int queue_pairs, i, has_cvq = 0; > + int queue_pairs, r, i, has_cvq = 0; > > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); > opts = &netdev->u.vhost_vdpa; > @@ -534,7 +539,12 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char > *name, > return -errno; > } > > - queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, > + r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp); > + if (unlikely(r < 0)) { > + return r; At this point in the code we have allocated the file descriptor vdpa_device_fd, but this return path fails to close it. > + } > + > + queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features, > &has_cvq, errp); > if (queue_pairs < 0) { > qemu_close(vdpa_device_fd); Compare this pre-existing error-exit path, which correctly calls qemu_close() on the fd. Related question: is this function supposed to return -1 on failure, or negative-errno ? At the moment it has a mix of both. I think that the sole caller only really wants "<0 on error", in which case the error-exit code paths could probably be tidied up so that instead of explicitly calling qemu_close() and returning r, queue_pairs, or whatever they got back from the function they just called, they could just 'goto err_svq' which will do the "close the fd and return -1" work. Better still, by initializing 'i' to 0 at the top of the function (and naming it something clearer, ideally), all the code paths after the initial qemu_open() succeeds could be made to use 'goto err' for the error-exit case. thanks -- PMM