Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains

[Paolo Bonzini <pbonzini@redhat.com>]

On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote:
> - First of all, inconsistency between block_job_create under
> aiocontext lock that internally calls blk_insert_bs and therefore
> bdrv_replace_child_noperm, and blk_insert_bs that is called two lines
> above in the same test without aiocontext. There seems to be no
> reason on why we need the lock there, so move the aiocontext lock further
> down.
> 
> - test_detach_indirect: here it is simply a matter of wrong callbacks
> used. In the original test, there was only a subtree drain, so
> overriding .drained_begin was not a problem. Now that we want to have
> additional subtree drains, we risk to call the test callback
> to early, or multiple times. We do not want that, so override
> the callback only when we actually want to use it.

The language here is a bit overcomplicated.  Don't think that you're 
writing Italian, instead use simple sentences.

First, the test is inconsistent about taking the AioContext lock when 
calling bdrv_replace_child_noperm.  bdrv_replace_child_noperm is reached 
in two places: from blk_insert_bs directly, and via block_job_create. 
Only the second does it with the AioContext lock taken, and there seems 
to be no reason why the lock is needed.  Move aio_context_acquire 
further down.  [Any reason why you don't move it even further down, to 
cover only job_start?]

Second, test_detach_indirect is only interested in observing the first 
call to .drained_begin.  In the original test, there was only a single 
subtree drain; however, with additional drains introduced in 
bdrv_replace_child_noperm, the test callback would be called too early 
and/or multiple times.  Override the callback only when we actually want 
to use it, and put back the original after it's been invoked.

This could also be split in two commits.

Paolo