Re: [PATCH v10 06/21] job: move and update comments from blockjob.c

[Kevin Wolf <kwolf@redhat.com>]

Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben:
> This comment applies more on job, it was left in blockjob as in the past
> the whole job logic was implemented there.
> 
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
> 
> No functional change intended.
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  blockjob.c | 20 --------------------
>  job.c      | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 4868453d74..7da59a1f1c 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -36,21 +36,6 @@
>  #include "qemu/main-loop.h"
>  #include "qemu/timer.h"
>  
> -/*
> - * The block job API is composed of two categories of functions.
> - *
> - * The first includes functions used by the monitor.  The monitor is
> - * peculiar in that it accesses the block job list with block_job_get, and
> - * therefore needs consistency across block_job_get and the actual operation
> - * (e.g. block_job_set_speed).  The consistency is achieved with
> - * aio_context_acquire/release.  These functions are declared in blockjob.h.
> - *
> - * The second includes functions used by the block job drivers and sometimes
> - * by the core block layer.  These do not care about locking, because the
> - * whole coroutine runs under the AioContext lock, and are declared in
> - * blockjob_int.h.
> - */
> -
>  static bool is_block_job(Job *job)
>  {
>      return job_type(job) == JOB_TYPE_BACKUP ||
> @@ -433,11 +418,6 @@ static void block_job_event_ready(Notifier *n, void *opaque)
>  }
>  
>  
> -/*
> - * API for block job drivers and the block layer.  These functions are
> - * declared in blockjob_int.h.
> - */
> -
>  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>                         JobTxn *txn, BlockDriverState *bs, uint64_t perm,
>                         uint64_t shared_perm, int64_t speed, int flags,
> diff --git a/job.c b/job.c
> index ae25db97ac..ebaa4e585b 100644
> --- a/job.c
> +++ b/job.c
> @@ -32,6 +32,20 @@
>  #include "trace/trace-root.h"
>  #include "qapi/qapi-events-job.h"
>  
> +/*
> + * The job API is composed of two categories of functions.
> + *
> + * The first includes functions used by the monitor.  The monitor is
> + * peculiar in that it accesses the block job list with job_get, and

s/block job/job/

> + * therefore needs consistency across job_get and the actual operation
> + * (e.g. job_user_cancel). To achieve this consistency, the caller
> + * calls job_lock/job_unlock itself around the whole operation.
> + *
> + *
> + * The second includes functions used by the block job drivers and sometimes

Same here.

> + * by the core block layer. These delegate the locking to the callee instead.
> + */

Unless I'm missing something, this comment (specifically the part with
calling job_lock/job_unlock outside of job.c) is actually not true at
this point in the series. I would suggest adding a comment to this
effect, like:

    * TODO Actually make this true

Then we know that when you remove the comment, we need to review that
it's actually true at that point in the series.

For now, I'll just try to remember checking this later.

Kevin