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

[Emanuele Giuseppe Esposito]

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