Re: [RFC PATCH v2 04/25] include/block/block: split header into I/O and global state API

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 07/10/2021 15:01, Stefan Hajnoczi wrote:
> On Thu, Oct 07, 2021 at 12:43:43PM +0100, Daniel P. Berrangé wrote:
>> On Tue, Oct 05, 2021 at 10:31:54AM -0400, Emanuele Giuseppe Esposito wrote:
>>> Similarly to the previous patch, split block.h
>>> in block-io.h and block-global-state.h
>>>
>>> block-common.h contains the structures shared between
>>> the two headers, and the functions that can't be categorized as
>>> I/O or global state.
>>
>> This is nice from a code organization POV, but it doesn't do all
>> that much from a code reviewer / author POV as I doubt anyone
>> will remember which header file the respective APIs/structures/
>> constants are in, without having to look it up each time.

Another thing to consider is with regards the function pointers API 
split I do in patches 19-25. There we don't even have a file separation,
so if someone misses the head comment separating I/O from GS, a new 
function can end up under the wrong API.

Maybe as Stefan suggests below we can at least split the function 
pointers in two separate structs, with bdrv_state_* and bdrv_io_* struct 
naming conventions.

But anyways thank you for the suggestions, I will add them to my TODO list.

Emanuele

>>
>> It would make life easier if we had distinct namning conventions
>> for APIs/struct/contsants in the respective headers.
>>
>> eg instead of  "bdrv_" have "bdrv_state_" and "bdrv_io_" as
>> the two naming conventions for -global-state.h and -io.h
>> respectively, nad only use the bare 'bdrv_' for -common.h
>>
>> Yes, this would be major code churn, but I think it'd make
>> the code clearer to understand which will be a win over the
>> long term.
>>
>> NB, I'm not suggesting doing a rename as part of this patch
>> though. Any rename would have to be separate, and likely
>> split over many patches to make it manageable.
> 
> Yes. Taking it one step further, BlockDriverState could be split into
> two struct so that I/O code doesn't even have access to the struct
> needed to invoke GS APIs. This is a type-safe way of enforcing the API
> split.
> 
> Unfortunately that's a lot of code churn and I think the separation is
> not very clean. For example, block drivers need to forward requests to
> their children, so they need to traverse the graph (which we think of as
> global state).
> 
> Stefan
>