Re: [Qemu-devel] [PATCH v6 06/14] blockjob: Add .commit and .abort block job actions

[John Snow <jsnow@redhat.com>]

On 09/15/2015 02:11 AM, Fam Zheng wrote:
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/block/blockjob.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 3e7ad21..a7b497c 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -50,6 +50,24 @@ typedef struct BlockJobDriver {
>       * manually.
>       */
>      void (*complete)(BlockJob *job, Error **errp);
> +
> +    /**
> +     * If the callback is not NULL, it will be invoked when all the jobs
> +     * belonging to the same transaction complete; or upon this job's
> +     * completion if it is not in a transaction. Skipped if NULL.
> +     *
> +     * Exactly one of .commit() and .abort() will be called for each job.
> +     */
> +    void (*commit)(BlockJob *job);
> +

I find this phrasing strange, but maybe it's just me. "Exactly one of
commit and abort will be called for each job" implies [to me] that it'd
be possible to call commit for one, but abort for different jobs [in a
transaction] -- but clearly we don't mean that. It is the "for each job"
that implies an iteration over a collection to me.

Just above we say "[commit] will be invoked when all the jobs belonging
to the same transaction are complete" which itself implies either all
jobs will be committed or all jobs will be aborted.

Maybe:

"All jobs will complete with a call to either .commit() or .abort() but
never both."

But I might be being too bikesheddy.

> +    /**
> +     * If the callback is not NULL, it will be invoked when any job in the
> +     * same transaction fails; or upon this job's failure (due to error or
> +     * cancellation) if it is not in a transaction. Skipped if NULL.
> +     *
> +     * Exactly one of .commit() and .abort() will be called for each job.
> +     */
> +    void (*abort)(BlockJob *job);
>  } BlockJobDriver;
>  
>  /**
> 

I'm probably just too picky.

Reviewed-by: John Snow <jsnow@redhat.com>