Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API

[Hanna Reitz]

On 16.11.21 15:24, Emanuele Giuseppe Esposito wrote:
> On 12/11/2021 13:30, Hanna Reitz wrote:
> > On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
> > > Similarly to the previous patches, split block-backend.h
> > > in block-backend-io.h and block-backend-global-state.h
> > >
> > > In addition, remove "block/block.h" include as it seems
> > > it is not necessary anymore, together with "qemu/iov.h"
> > >
> > > block-backend-common.h contains the structures shared between
> > > the two headers, and the functions that can't be categorized as
> > > I/O or global state.
> > >
> > > Assertions are added in the next patch.
> > >
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >   block/block-backend.c                       |   9 +-
> > >   include/sysemu/block-backend-common.h       |  74 ++++++
> > >   include/sysemu/block-backend-global-state.h | 122 +++++++++
> > >   include/sysemu/block-backend-io.h           | 139 ++++++++++
> > >   include/sysemu/block-backend.h              | 269 
> > > +-------------------
> > >   5 files changed, 344 insertions(+), 269 deletions(-)
> > >   create mode 100644 include/sysemu/block-backend-common.h
> > >   create mode 100644 include/sysemu/block-backend-global-state.h
> > >   create mode 100644 include/sysemu/block-backend-io.h
> >
> > [...]
> >
> > > diff --git a/include/sysemu/block-backend.h 
> > > b/include/sysemu/block-backend.h
> > > index e5e1524f06..038be9fc40 100644
> > > --- a/include/sysemu/block-backend.h
> > > +++ b/include/sysemu/block-backend.h
> > > @@ -13,272 +13,9 @@
> > >   #ifndef BLOCK_BACKEND_H
> > >   #define BLOCK_BACKEND_H
> > > -#include "qemu/iov.h"
> > > -#include "block/throttle-groups.h"
> > > +#include "block-backend-global-state.h"
> > > +#include "block-backend-io.h"
> > > -/*
> > > - * TODO Have to include block/block.h for a bunch of block layer
> > > - * types.  Unfortunately, this pulls in the whole BlockDriverState
> > > - * API, which we don't want used by many BlockBackend users. Some of
> > > - * the types belong here, and the rest should be split into a common
> > > - * header and one for the BlockDriverState API.
> > > - */
> > > -#include "block/block.h"
> >
> > This note and the include is gone.  Sounds like something positive, 
> > but why is this possible?
>
> Basically block/throttle-groups.h includes block/block_int.h that 
> internally includes block/block.h.
>
> But I am not sure if you actually want to keep this comment as 
> reminder for future work. Should I keep it?

Good question.  I think I’d keep it and the block.h include; I mean, the 
throttle-groups.h include was there before already, so perhaps this was 
indeed only intended as a reminder.

The other reason to keep it is that ideal this is just a refactoring 
patch, so I wouldn’t touch anything that needn’t be touched.

Hanna