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

[Hanna Reitz <hreitz@redhat.com>]

On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
> Introduce .pre_run() job callback. This cb will run in job_start,
> before the coroutine is created and runs run() in the job aiocontext.

I presume this means “before the coroutine is created that will run 
run() in the job aiocontext”?

(The way it’s written here sounds a bit like .pre_run() will run run())

> 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().
>
> In addition, doing so we check for permissions in all bdrv
> in amend, not only crypto.

As I’ve written in my reply to patch 30, I’m not quite sold on this 
part, but disregarding that part (and one typo below), this patch looks 
good to me.

> .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>
> ---
>   block/amend.c      | 33 +++++++++++++++++++++++++++++++++
>   include/qemu/job.h |  9 +++++++++
>   2 files changed, 42 insertions(+)

[...]

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 4ea7a4a0cd..915ceff425 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -223,6 +223,15 @@ struct JobDriver {
>        * the GS API.
>        */
>   
> +    /**
> +     * Called in the Main Loop context, before the job coroutine
> +     * is started in the job aiocontext. Useful to perform operations
> +     * that needs to be done under BQL (like permissions setup).

s/needs/need/

> +     * On success, it returns 0. Otherwise the job failed to initialize
> +     * and won't start.
> +     */
> +    int (*pre_run)(Job *job, Error **errp);
> +
>       /**
>        * Called when the job is resumed by the user (i.e. user_paused becomes
>        * false). .user_resume is called before .resume.