On Mon, Aug 1, 2022 at 5:29 AM Jason Wang <jasow...@redhat.com> wrote: > > > 在 2022/7/29 22:08, Peter Maydell 写道: > > 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. > > > Exactly. >
Right, I'll fix. > > > > >> + } > >> + > >> + 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 ? > > > Kind of either: > > if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) > < 0) { > /* FIXME drop when all init functions store an Error */ > if (errp && !*errp) { > error_setg(errp, "Device '%s' could not be initialized", > NetClientDriver_str(netdev->type)); > } > return -1; > } > > We can write errno to errp then, and consistently use the goto for error handling as you propose. I'll post a fix in a moment. Thanks! > > 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. > > > Yes, having a consistent goto based error handling seems much better. > > Eugenio, please post patch to fix this. > > Thanks > > > > > > thanks > > -- PMM > > >