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

[John Snow <jsnow@redhat.com>]

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 <jsnow@redhat.com>
>
>> --- 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