Re: [PATCH] vhost-net: fix reversed logic in mask notifiers

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > When guest notifier is assigned, we set mask notifier,
> > which will assign kvm irqfd.
> > When guest notifier is unassigned, mask notifier is unset,
> > which should unassign kvm irqfd.
> >
> > The way to do this is to call mask notifier telling it to mask the vector.
> > This, unless vector is already masked which unassigns irqfd already.
> >
> > The logic in unassign was reversed, which left kvm irqfd assigned.
> >
> > This patch is qemu-kvm only as irqfd is not upstream.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Reported-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/msix.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 8f9a621..1398680 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -617,6 +617,7 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
> >      assert(opaque);
> >      assert(!dev->msix_mask_notifier_opaque[vector]);
> >  
> > +    /* Unmask the new notifier unless vector is masked. */
> >      if (msix_is_masked(dev, vector)) {
> >          return 0;
> >      }
> > @@ -638,12 +639,13 @@ int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
> >      assert(dev->msix_mask_notifier);
> >      assert(dev->msix_mask_notifier_opaque[vector]);
> >  
> > +    /* Mask the old notifier unless it is already masked. */
> >      if (msix_is_masked(dev, vector)) {
> >          return 0;
> >      }
> >      r = dev->msix_mask_notifier(dev, vector,
> >                                  dev->msix_mask_notifier_opaque[vector],
> > -                                msix_is_masked(dev, vector));
> > +                                !msix_is_masked(dev, vector));
> 
> Why don't put just a 1 here?
> 
> we have:
> 
> if (msix_is_masked())
>    return 0
> r = msix_mask_notifier(....., !msix_is_masked());
> 
> i.e. at that point msix_is_masked() is false, or we really, really needs
> locking.
> 
> Puttting a !foo, when we know that it needs to be an 1 looks strange.
> 
> Later, Juan.
> 
> PD.  Yes, I already asked in a previous version to just have two
> methods, mask/unmask.  we now at call time which one we need.


I find msix_is_masked clearer here than true since you don't need
to look up definition to understand what this 'true' stands for.
The value is clear from code above. What do you think?

-- 
MST