Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init()

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, May 15, 2017 at 09:14:33PM +0800, Peter Xu wrote:
>> MSI should be supported by all interrupt controllers. Switching the old
>> check for msi_nonbroken into assertion. Do similar thing to
>> pci_add_capability2() below that. Then time to remove *errp.
>> 
>> Since msi_init() won't fail now, touch up all the callers to avoid
>> checks against it. One side effect is that we fixed a possible leak in
>> current edu device.
>> 
>> Reported-by: Markus Armbruster <armbru@redhat.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  hw/audio/intel-hda.c               | 18 +-----------------
>>  hw/i386/amd_iommu.c                |  2 +-
>>  hw/ide/ich.c                       |  6 +-----
>>  hw/misc/edu.c                      |  4 +---
>>  hw/net/e1000e.c                    |  6 +-----
>>  hw/net/trace-events                |  1 -
>>  hw/net/vmxnet3.c                   |  8 ++------
>>  hw/pci-bridge/ioh3420.c            | 17 ++++-------------
>>  hw/pci-bridge/pci_bridge_dev.c     | 19 +------------------
>>  hw/pci-bridge/xio3130_downstream.c | 11 +++--------
>>  hw/pci-bridge/xio3130_upstream.c   | 11 +++--------
>>  hw/pci/msi.c                       | 25 ++++++-------------------
>>  hw/scsi/megasas.c                  | 18 +-----------------
>>  hw/scsi/mptsas.c                   | 20 ++------------------
>>  hw/scsi/trace-events               |  1 -
>>  hw/scsi/vmw_pvscsi.c               | 12 +++---------
>>  hw/usb/hcd-xhci.c                  | 16 +---------------
>>  hw/vfio/pci.c                      | 13 ++-----------
>>  include/hw/pci/msi.h               |  6 +++---
>>  19 files changed, 36 insertions(+), 178 deletions(-)
>
> Ping?
>
> Just to mention in case missed - this is also a bug fix for edu
> device.
>
> Also CC Markus since he's the reporter and I forgot to CC him in
> previous post. Sorry.

The patch indeed fixes the leak in the edu device.  It might fix similar
cleanup errors in other devices; I didn't check.

The interesting part is of course having devices assert the interrrupt
controller isn't broken.  The commit message claims "MSI should be
supported by all interrupt controllers".  Does that mean "you think it
is supported", or does it mean "it really should be supported"?

If the former, shouldn't we drop @msi_nonbroken entirely?

If the latter, why is it okay to assert?

The Suggested-by makes me suspect this has been explained elsewhere
already; feel free to send me just a pointer.