On Tue, 16 Apr 2024 at 13:41, Cindy Lu <l...@redhat.com> wrote:
>
> On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.mayd...@linaro.org>
> wrote:
> >
> > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <l...@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
> > >
> > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > > Cc: qemu-sta...@nongnu.org
> > > Signed-off-by: Cindy Lu <l...@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 ?
> 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
