Re: [PATCH v6 31/33] include/qemu/job.h: introduce job->pre_run() and use it in amend

[Emanuele Giuseppe Esposito]

On 07/02/2022 19:14, Kevin Wolf wrote:
> Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
>> Introduce .pre_run() job callback. This cb will run in job_start,
>> before the coroutine is created and runs run() in the job aiocontext.
>>
>> Therefore, .pre_run() always runs in the main loop.
>> We can use this function together with clean() cb to replace
>> bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks(),
>> since that function can also be called from an iothread via
>> .bdrv_co_amend().
> 
> How is this different from having the same code in the function that
> creates the job, i.e. qmp_x_blockdev_amend()?
> 
> Almost all block jobs have some setup code in the function that creates
> the job instead of doing everything in .run(), precisely because they
> know this code runs in the main thread.
> 
> Is amend really so different from the other block jobs in this respect
> that it needs a different solution?
> 

Are you suggesting to simply call .bdrv_amend_pre_run before job_start()
and just leave JobDriver .clean() to call .bdrv_amend_clean?

Yes, that will work too. I will delete .pre_run().

>> In addition, doing so we check for permissions in all bdrv
>> in amend, not only crypto.
>>
>> .pre_run() and .clean() take care of calling bdrv_amend_pre_run()
>> and bdrv_amend_clean() respectively, to set up driver-specific flags
>> and allow the crypto driver to temporarly provide the WRITE
>> perm to qcrypto_block_amend_options().
>>
>> .pre_run() is not yet invoked by job_start, but .clean() is.
>> This is not a problem, since it will just be a redundant check
>> and crypto will have the update->keys flag == false anyways.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> I find the way how you split the patches a bit confusing because the
> patches aren't self-contained, but always refer to what the code will do
> in the future, because after the patch it's dead code that isn't even
> theoretically called until the final patch comes in.
> 
> Can we restructure this a bit? First a patch that adds a new JobDriver
> callback (if really needed) along with the actual calls for it and
> everything else that needs to be touched in the generic job
> infrastructure. Second, new BlockDriver callbacks with all of the
> plumbing code. Third, the amend job changes with a patch that doesn't
> touch anything but block/amend.c and potentially block/crypto.c (the
> latter could also be another separate patch).

It is more or less what also Hanna suggested, I have it for the next
version.
> 
> This change with three or four patches could also be a candidate to be
> split out into a separate smaller series.

Makes sense.

Emanuele
> 
> Kevin
>