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

[Michael S. Tsirkin]

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.

> > 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