Re: [PULL V2 19/25] vdpa: Extract get features part from vhost_vdpa_get_max_queue_pairs

[Eugenio Perez Martin]

On Mon, Aug 1, 2022 at 5:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/7/29 22:08, Peter Maydell 写道:
> > On Wed, 20 Jul 2022 at 10:04, Jason Wang <jasowang@redhat.com> wrote:
> >> From: Eugenio Pérez <eperezma@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 <eperezma@redhat.com>
> >> Acked-by: Jason Wang <jasowang@redhat.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@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
> >
>