Re: [Qemu-devel] [PATCH v7 03/10] pci: Convert msix_init() to Error and fix callers to check it

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/01/2017 18:54, Michael S. Tsirkin wrote:
>> On Mon, Nov 14, 2016 at 03:25:33PM +0800, Cao jin wrote:
>>> msix_init() reports errors with error_report(), which is wrong when
>>> it's used in realize().  The same issue was fixed for msi_init() in
>>> commit 1108b2f.
>>>
>>> For some devices(like e1000e, vmxnet3) who won't fail because of
>>> msix_init's failure, suppress the error report by passing NULL error object.
>>>
>>> Bonus: add comment for msix_init.
>>>
>>> CC: Jiri Pirko <jiri@resnulli.us>
>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>> CC: Dmitry Fleytman <dmitry@daynix.com>
>>> CC: Jason Wang <jasowang@redhat.com>
>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>> CC: Hannes Reinecke <hare@suse.de>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> CC: Alex Williamson <alex.williamson@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> 
>> I'd rather add a new API. Once that is merged you can make device
>> changes avoiding a flag day. Merge this through individual trees. At the
>> end of the day we can drop the old API when it's no longer needed.
>
> I think that's the worst.  We don't need another partial transition and

Seconded.

> this series is all but big.  The main issue is that it was handled badly
> in the past, so people tend not to trust it.
>
> If anything, if there are independent patches in the series that can be
> merged through USB or SCSI trees, before this one, we can do that.

I guess some of the later patches are follow-up cleanups that could be
merged separately.  Might require reordering, then re-review.  But would
this really be worth the trouble?  Merging through another tree is no
pixie dust for quality.  If we can get the review and testing we need
only by merging every shard through the exact right tree, we're doing it
wrong.

Yes, we absolutely want a maintainer to review patches, and yes, that's
not trivial for work spanning subsystems.  But we got the R-bys alright.

Elsewhere in this thread, you ask for specific testing in addition to a
maintainer's R-by.  I think that's fair.