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 <eespo...@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
>
