Re: [Qemu-devel] [PATCH 05/10] blockjob: separate monitor and blockjob APIs

[John Snow <jsnow@redhat.com>]

On 04/11/2017 12:57 AM, Paolo Bonzini wrote:
> 
> 
> On 11/04/2017 00:05, John Snow wrote:
>>
>>
>> On 04/08/2017 05:52 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 08/04/2017 08:03, John Snow wrote:
>>>> Looks clean, though it may be useful to do a few more things;
>>>>
>>>> - Demarcate what you think is the monitor API in this file
>>>
>>> It's already there:
>>>
>>> +/*
>>> + * API for block job drivers and the block layer.
>>> + */
>>> +
>>>
>>> where everything before is for the monitor.
>>>
>>
>> I meant explicitly, with a comment at the top explaining the demarcation.
> 
> Oh, sure.
> 
>>>> - Organize blockjob.h to match to serve as a useful reference.
>>>
>>> Hmm, yes.
> 
> As it turns out, no headers are necessary---but yours was a very good
> remark still, because a couple mistakes in this series stood out when
> checking.
> 
> The "API for block job drivers and the block layer" is already in
> blockjob_int.h while the rest is in blockjob.h.  This is nice because it
> provides some validation of the concept behind the patch, and also of
> the locking policy I chose for the rest of the work.
> 
> But, there are two exceptions.  Both of them are introduced by this
> series and they shouldn't be:
> 
> - blockjob_pause/resume_all should have its declaration in block_int.h,
> so I've fixed patch 4 accordingly
> 
> - blockjob_create is in blockjob_int.h, but this patch should move it to
> the second part of the file, too.
> 
> Thanks,
> 
> Paolo
> 

"You were wrong, but so was I, thanks for being wrong in a helpful way."

No problem.

--js