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? > 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). This change with three or four patches could also be a candidate to be split out into a separate smaller series. Kevin
