On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> > You could always keep the functions as separate wrapper callers of the
> > common function so you only need to keep true = unset, false = set
> > straight in one place. Thanks,
>
>
> Just to show why it does not work, I did exactly this: as you see the
> code is shorter but the true/false magic gets spread: it was in 2
> places, (set/unset) now it is in 4 places and it is within the loop, in
> code that is more complex.
You seem to have missed the wrapper function. I'm simply suggesting
something like this:
static int __do_msix_mask_notifier_for_vector(PCIDevice *dev, unsigned vector, bool mask)
{
if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
return 0;
assert(dev->msix_mask_notifier);
/* Set the new notifier unless vector is masked. */
if (!msix_is_masked(dev, vector)) {
return dev->msix_mask_notifier(dev, vector, mask);
}
return 0;
}
static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
return __do_msix_mask_notifier_for_vector(dev, vector, false);
}
static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
return __do_msix_mask_notifier_for_vector(dev, vector, true);
}
Which then doesn't go on to complicate the callers like the below does.
Thanks,
Alex
> So I think I'll stick to the original version and we can
> patch it up later if there's a will.
>
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 3d4dd61..4b705a0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -583,40 +583,15 @@ void msix_unuse_all_vectors(PCIDevice *dev)
> msix_free_irq_entries(dev);
> }
>
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +/* Invoke the notifier if vector entry is used and unmasked. */
> +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, bool masked)
> {
> int r = 0;
> - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> + if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
> return 0;
> -
> - assert(dev->msix_mask_notifier);
> -
> - /* Unmask the new notifier unless vector is masked. */
> - if (!msix_is_masked(dev, vector)) {
> - r = dev->msix_mask_notifier(dev, vector, false);
> - if (r < 0) {
> - return r;
> - }
> }
> - return r;
> -}
> -
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> -{
> - int r = 0;
> - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> - return 0;
> -
> assert(dev->msix_mask_notifier);
> -
> - /* Mask the old notifier unless it is already masked. */
> - if (!msix_is_masked(dev, vector)) {
> - r = dev->msix_mask_notifier(dev, vector, true);
> - if (r < 0) {
> - return r;
> - }
> - }
> - return r;
> + return dev->msix_mask_notifier(dev, vector, masked);
> }
>
> int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> @@ -625,7 +600,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> assert(!dev->msix_mask_notifier);
> dev->msix_mask_notifier = f;
> for (n = 0; n < dev->msix_entries_nr; ++n) {
> - r = msix_set_mask_notifier_for_vector(dev, n);
> + r = msix_notify_if_unmasked(dev, n, false);
> if (r < 0) {
> goto undo;
> }
> @@ -634,7 +609,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>
> undo:
> while (--n >= 0) {
> - msix_unset_mask_notifier_for_vector(dev, n);
> + msix_notify_if_unmasked(dev, n, true);
> }
> dev->msix_mask_notifier = NULL;
> return r;
> @@ -645,7 +620,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
> int r, n;
> assert(dev->msix_mask_notifier);
> for (n = 0; n < dev->msix_entries_nr; ++n) {
> - r = msix_unset_mask_notifier_for_vector(dev, n);
> + r = msix_notify_if_unmasked(dev, n, true);
> if (r < 0) {
> goto undo;
> }
> @@ -655,7 +630,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>
> undo:
> while (--n >= 0) {
> - msix_set_mask_notifier_for_vector(dev, n);
> + msix_notify_if_unmasked(dev, n, false);
> }
> return r;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
