Re: [Qemu-devel] [PATCH 08/42] job: Create Job, JobDriver and job_create()

[Max Reitz]

On 2018-05-09 18:26, Kevin Wolf wrote:
> This is the first step towards creating an infrastructure for generic
> background jobs that aren't tied to a block device. For now, Job only
> stores its ID and JobDriver, the rest stays in BlockJob.
> 
> The following patches will move over more parts of BlockJob to Job if
> they are meaningful outside the context of a block job.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/block/blockjob.h     |  9 +++----
>  include/block/blockjob_int.h |  4 +--
>  include/qemu/job.h           | 60 
> ++++++++++++++++++++++++++++++++++++++++++++
>  block/backup.c               |  4 ++-
>  block/commit.c               |  4 ++-
>  block/mirror.c               | 10 +++++---
>  block/stream.c               |  4 ++-
>  blockjob.c                   | 46 ++++++++++++++++-----------------
>  job.c                        | 48 +++++++++++++++++++++++++++++++++++
>  tests/test-bdrv-drain.c      |  4 ++-
>  tests/test-blockjob-txn.c    |  4 ++-
>  tests/test-blockjob.c        | 12 ++++++---
>  MAINTAINERS                  |  2 ++
>  Makefile.objs                |  2 +-
>  14 files changed, 169 insertions(+), 44 deletions(-)
>  create mode 100644 include/qemu/job.h
>  create mode 100644 job.c
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 0b57d53084..8acc1a236a 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h

[...]

> @@ -40,6 +41,9 @@ typedef struct BlockJobTxn BlockJobTxn;
>   * Long-running operation on a BlockDriverState.
>   */
>  typedef struct BlockJob {
> +    /** Data belonging to the generic Job infrastructure */
> +    Job job;
> +
>      /** The job type, including the job vtable.  */
>      const BlockJobDriver *driver;

Any reason why you keep this field around?  Shouldn't it be just
DO_UPCAST(const BlockJobDriver, job_driver, job.driver)?

>  
> @@ -47,11 +51,6 @@ typedef struct BlockJob {
>      BlockBackend *blk;
>  
>      /**
> -     * The ID of the block job. May be NULL for internal jobs.
> -     */
> -    char *id;
> -
> -    /**
>       * The coroutine that executes the job.  If not NULL, it is
>       * reentered when busy is false and the job is cancelled.
>       */

[...]

> diff --git a/blockjob.c b/blockjob.c
> index 9943218a34..35d604d05a 100644
> --- a/blockjob.c
> +++ b/blockjob.c

[...]

> @@ -247,7 +247,7 @@ void block_job_unref(BlockJob *job)
>                                          block_job_detach_aio_context, job);
>          blk_unref(job->blk);
>          error_free(job->blocker);
> -        g_free(job->id);
> +        g_free(job->job.id);

Err.  OK.  I put my faith in patch 11.

Reviewed-by: Max Reitz <address@hidden>

Although I do wonder about BlockJob.driver.  It seems to stay even after
this series...

>          assert(!timer_pending(&job->sleep_timer));
>          g_free(job);
>      }

[...]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 459e3594e1..a97f60d104 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1369,6 +1369,8 @@ L: address@hidden

(Cut off here:

M: Jeff Cody <address@hidden>

Clever!)

>  S: Supported
>  F: blockjob.c
>  F: include/block/blockjob.h
> +F: job.c
> +F: include/block/job.h
>  F: block/backup.c
>  F: block/commit.c
>  F: block/stream.c

signature.asc
Description: OpenPGP digital signature