Re: [Qemu-block] [PATCH v2 05/11] blockjobs: split interface into public/private

[John Snow]

On 10/05/2016 10:17 AM, Kevin Wolf wrote:
> Am 01.10.2016 um 00:00 hat John Snow geschrieben:
> > To make it a little more obvious which functions are intended to be
> > public interface and which are intended to be for use only by jobs
> > themselves, split the interface into "public" and "private" files.
> >
> > Convert blockjobs (e.g. block/backup) to using the private interface.
> > Leave blockdev and others on the public interface.
> >
> > Give up and let qemu-img use the internal interface, though it doesn't
> > strictly need to be using it.
> >
> > As a side-effect, hide the BlockJob and BlockJobDriver implementation
> > from most of the QEMU codebase.
> >
> > Signed-off-by: John Snow <address@hidden>
>
> > --- a/block/replication.c
> > +++ b/block/replication.c
> > @@ -15,7 +15,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu-common.h"
> >  #include "block/nbd.h"
> > -#include "block/blockjob.h"
> > +#include "block/blockjob_int.h"
> >  #include "block/block_int.h"
> >  #include "block/block_backup.h"
> >  #include "sysemu/block-backend.h"
>
> This one is wrong. replication.c is a block job user, not part of the
> implementation. After removing this hunk, the result still compiles.

Sorry, this is a mistake from an intermediate form of the patch.

> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -7,16 +7,15 @@
> >  #include "qemu/coroutine.h"
> >  #include "block/accounting.h"
> >  #include "block/dirty-bitmap.h"
> > +#include "block/blockjob.h"
> >  #include "qapi/qmp/qobject.h"
> >  #include "qapi-types.h"
> >  #include "qemu/hbitmap.h"
>
> Hm... This header file doesn't need any of the definitions from
> blockjob.h, so I guess .c files should include it explicitly rather than
> getting it from here. That would be a separate patch, though.

OK. The reason this happened was because we used to have the struct 
definitions in block_int.h; and by moving those to a Blockjob-specific 
header, I needed to reintroduce those definitions somehow. I reasoned 
that BlockJobs were part of the public interface for the Block layer, so 
I added it here.

I can remove it and include the public blockjob interface only where 
specifically required if desired, though.

> >  /* block.c */
> >  typedef struct BlockDriver BlockDriver;
> > -typedef struct BlockJob BlockJob;
> >  typedef struct BdrvChild BdrvChild;
> >  typedef struct BdrvChildRole BdrvChildRole;
> > -typedef struct BlockJobTxn BlockJobTxn;
> >
> >  typedef struct BlockDriverInfo {
> >      /* in bytes, 0 if irrelevant */
>
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -37,7 +37,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/block-backend.h"
> >  #include "block/block_int.h"
> > -#include "block/blockjob.h"
> > +#include "block/blockjob_int.h"
> >  #include "block/qapi.h"
> >  #include "crypto/init.h"
> >  #include "trace/control.h"
>
> qemu-img doesn't compile without including blockjob_int.h, but that's
> just a sign that the split isn't completely right yet. Maybe it needs to
> use the pulic function block_job_query() to get the information it takes
> directly from the BlockJob struct.
>
> Kevin

Yes, you caught me. I'd need to spend a little more time shining up 
qemu-img, which just takes time away from that FDC rev-- I'm kidding. 
I'll fix this up at your request.

--js