Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Paolo Bonzini]

On 21/06/2016 12:56, Kevin Wolf wrote:
> Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
>>
>>
>> On 21/06/2016 11:21, Kevin Wolf wrote:
>>> This series converts all I/O function in the core block layer up to
>>> bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
>>> instead of a BlockDriverState.
>>>
>>> The original motivation for this change were op blockers, where one of
>>> the biggest problems is making sure that every user of block devices
>>> actually registers correctly with the op blockers system. If the I/O
>>> functions know which parent a request comes from (BdrvChild basically
>>> corresponds to an edge in our block device graph), it can use assertions
>>> to make sure that that parent has actually registered its activities and
>>> thereby ensured that it doesn't conflict with other users.
>>>
>>> There are, however, more benefits we get from this change. The most
>>> important one is probably that it enforces important aspects of the
>>> block layer design like that external users go through a BlockBackend
>>> and request are internally routed along the edges of the graph. Accesses
>>> to random BDSes are no longer possible, you need to own an actual child
>>> reference so you can make a request.
>>
>> I still fail to understand what is the rationale for this change.  The
>> API is weird; you read from a disk, not from an edge, and in fact the
>> first thing all the APIs do is dereference the BdrvChild...
>>
>> The assertions are nice, but I'm not sure it's a good idea to design a
>> whole API around them.
> 
> Do you see a problem with such an API, though? If there is no reason not
> to have the advantages, as small as they may seem, why not take them?

I don't see a reason not to take them; I don't see any red flags, but
there are some yellow flags (the kinda weird API) that I don't
understand and I hope you can explain.

Thinking more about it, it's perfectly possible that this is just a
combination of block/io.c's growth by accretion and the well-known fact
"naming pseudo-OOP member functions in C sucks".

In other words, if you sell me this as "let's add some member functions
to BdrvChild and use them", I can buy it.  Perhaps the only thing to do
then is to rename functions and design a consistent naming.

Paolo