Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Cindy Lu]

On Wed, Apr 17, 2024 at 2:38 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> > On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> 
> > > wrote:
> > > >
> > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
> > > > >
> > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label,
> > > > > the function will get the vector incorrectly while using
> > > > > VIRTIO_CONFIG_IRQ_IDX
> > > > > To fix this, we remove this label and simplify the failure process
>
> And then what happens?  It's unclear whether it's a real or
> theoretical issue.
>
> > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > ---
> > > > >  hw/virtio/virtio-pci.c | 19 +++----------------
> > > > >  1 file changed, 3 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > index b138fa127a..565bdb0897 100644
> > > > > --- a/hw/virtio/virtio-pci.c
> > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > @@ -892,7 +892,7 @@ static int 
> > > > > kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > > >      }
> > > > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > > > >      if (ret < 0) {
> > > > > -        goto undo;
> > > > > +        return ret;
> > > > >      }
> > > > >      /*
> > > > >       * If guest supports masking, set up irqfd now.
> > > > > @@ -902,25 +902,12 @@ static int 
> > > > > kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > > >          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> > > > >          if (ret < 0) {
> > > > >              kvm_virtio_pci_vq_vector_release(proxy, vector);
> > > > > -            goto undo;
> > > > > +            kvm_virtio_pci_irqfd_release(proxy, n, vector);
> > > >
> > > > Are you sure this is right? The kvm_virtio_pci_irqfd_use()
> > > > just failed, so why do we need to call
> > > > kvm_virtio_pci_irqfd_release() ?
> >
> > > This version should be correct.  when kvm_virtio_pci_irqfd_use() fail
> > > we need to call kvm_virtio_pci_vq_vector_release() and
> > > kvm_virtio_pci_irqfd_release()
> > > but for kvm_virtio_pci_vq_vector_use fail we can simple return,
> >
> > But *why* do we need to call kvm_virtio_pci_irqfd_release()?
> >
> > In most API designs, this kind of pairing of "get/use/allocate
> > something" and "free/release something" function only
> > requires you to do the "release" if the "get" succeeded,
> > because if the "get" fails it's supposed to fail in way that
> > means "I didn't do anything". Is this API not following that
> > standard pattern ?
>
>
> I am just as puzzled.
>
got it. I made a mistake here, I will send the new version
Thanks
cindy
> > > in old version there is a error in failure process.
> > > while the kvm_virtio_pci_vq_vector_use fail it  call the
> > > kvm_virtio_pci_irqfd_release,but at this time this is irqfd
> > > is not using now
> >
> > thanks
> > -- PMM
>