On Mon, Aug 01, 2022 at 08:48:41PM +0200, Eugenio Perez Martin wrote: > On Mon, Aug 1, 2022 at 8:34 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > > > > On Mon, 1 Aug 2022 at 19:31, Laurent Vivier <lviv...@redhat.com> wrote: > > > > > > On 01/08/2022 16:47, Eugenio Pérez wrote: > > > > File descriptor vdpa_device_fd is not free in the case of returning > > > > error from vhost_vdpa_get_features. Fixing it by making all errors go to > > > > the same error path. > > > > > > > > Resolves: Coverity CID 1490785 > > > > Fixes: 8170ab3f43 ("vdpa: Extract get features part from > > > > vhost_vdpa_get_max_queue_pairs") > > > > > > > > Signed-off-by: Eugenio Pérez <epere...@redhat.com> > > > > --- > > > > net/vhost-vdpa.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > > > index 6abad276a6..303447a68e 100644 > > > > --- a/net/vhost-vdpa.c > > > > +++ b/net/vhost-vdpa.c > > > > @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const > > > > char *name, > > > > g_autofree NetClientState **ncs = NULL; > > > > g_autoptr(VhostIOVATree) iova_tree = NULL; > > > > NetClientState *nc; > > > > - int queue_pairs, r, i, has_cvq = 0; > > > > + int queue_pairs, r, i = 0, has_cvq = 0; > > > > > > > > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); > > > > opts = &netdev->u.vhost_vdpa; > > > > @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const > > > > char *name, > > > > > > > > r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp); > > > > if (unlikely(r < 0)) { > > > > - return r; > > > > + goto err; > > > > > > Why don't you use "goto err_svq"? > > > > This patch is kind of half of the idea discussed in the thread where > > this problem was reported, which is that by setting i = 0 you can > > then consistently have all the error handling be 'goto err' and that > > frees everything that needs to be freed regardless of whether it's > > called after or before the initialization of the ncs[] entries. But it > > doesn't do the other half of the job, which is making all the other > > error handling code in the function also use 'goto err', so it looks > > a bit odd as it stands. > > > > That's right, I thought just fixing the issue about the leaked file > descriptor was the right thing to do in the hard feature freeze, and > that other part should be left for the next dev phase. Is the unified > error handling code acceptable for this period? I can send a patch > either on top of this one or squashed if so for sure. > > Thanks!
Yea I'd prefer a minimal patch, unless others object strongly. -- MST