On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> 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
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-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 ?
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
