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

[Kevin Wolf <kwolf@redhat.com>]

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 <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).

This change with three or four patches could also be a candidate to be
split out into a separate smaller series.

Kevin