Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver

[Hanna Reitz]

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
> The job API will be handled separately in another serie.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/qemu/job.h | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 6e67b6977f..7e9e59f4b8 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -169,12 +169,21 @@ typedef struct Job {
>    * Callbacks and other information about a Job driver.
>    */
>   struct JobDriver {
> +
> +    /* Fields initialized in struct definition and never changed. */

Like in patch 19, I’d prefer a slightly more verbose comment that I’d 
find more easily readable.

> +
>       /** Derived Job struct size */
>       size_t instance_size;
>   
>       /** Enum describing the operation */
>       JobType job_type;
>   
> +    /*
> +     * Functions run without regard to the BQL and may run in any

s/and/that/?

> +     * arbitrary thread. These functions do not need to be thread-safe
> +     * because the caller ensures that are invoked from one thread at time.

s/that/they/ (or “that they”)

I believe .run() must be run in the job’s context, though.  Not sure if 
that’s necessary to note, but it isn’t really an arbitrary thread, and 
block jobs certainly require this (because they run in the block 
device’s context).  Or is that something that’s going to change with I/O 
threading?

Hanna