Re: [Qemu-devel] [PATCH 04/11] block: add refcount to Job object

[Max Reitz <mreitz@redhat.com>]

On 2015-03-04 at 23:15, John Snow wrote:
> If we want to get at the job after the life of the job,
> we'll need a refcount for this object.
>
> This may occur for example if we wish to inspect the actions
> taken by a particular job after a transactional group of jobs
> runs, and further actions are required.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockjob.c               | 18 ++++++++++++++++--
>   include/block/blockjob.h | 21 +++++++++++++++++++++
>   2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index ba2255d..d620082 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -35,6 +35,19 @@
>   #include "qemu/timer.h"
>   #include "qapi-event.h"
>   
> +void block_job_incref(BlockJob *job)
> +{
> +    job->refcount++;
> +}
> +
> +void block_job_decref(BlockJob *job)
> +{
> +    job->refcount--;
> +    if (job->refcount == 0) {
> +        g_free(job);
> +    }
> +}
> +
>   void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>                          int64_t speed, BlockCompletionFunc *cb,
>                          void *opaque, Error **errp)
> @@ -57,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>       job->cb            = cb;
>       job->opaque        = opaque;
>       job->busy          = true;
> +    job->refcount      = 1;
>       bs->job = job;
>   
>       /* Only set speed when necessary to avoid NotSupported error */
> @@ -68,7 +82,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>               bs->job = NULL;
>               bdrv_op_unblock_all(bs, job->blocker);
>               error_free(job->blocker);
> -            g_free(job);
> +            block_job_decref(job);
>               error_propagate(errp, local_err);
>               return NULL;
>           }
> @@ -85,7 +99,7 @@ void block_job_completed(BlockJob *job, int ret)
>       bs->job = NULL;
>       bdrv_op_unblock_all(bs, job->blocker);
>       error_free(job->blocker);
> -    g_free(job);
> +    block_job_decref(job);
>   }
>   
>   void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index b6d4ebb..8fe1c95 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -116,6 +116,9 @@ struct BlockJob {
>   
>       /** The opaque value that is passed to the completion function.  */
>       void *opaque;
> +
> +    /** A reference count, allowing for post-job actions in e.g. transactions */
> +    int refcount;
>   };
>   
>   /**
> @@ -141,6 +144,24 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>                          void *opaque, Error **errp);
>   
>   /**
> + * block_job_incref:
> + * @job: The job to pick up a handle to
> + *
> + * Increment the refcount on @job, to be able to use it asynchronously
> + * from the job it is being used for. Put down the reference when done
> + * with @block_job_unref

Missing a full stop?

With that added:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> + */
> +void block_job_incref(BlockJob *job);
> +
> +/**
> + * block_job_decref:
> + * @job: The job to unreference and delete.
> + *
> + * Decrement the refcount on @job, and delete it if there are no users.

(and also with that changed to "if it becomes/reaches zero", if you'd 
like, because somehow the "users" sounds unrelated to a party holding a 
refcount, when in fact it's supposed to be exactly the same thing; up to 
you)

> + */
> +void block_job_decref(BlockJob *job);
> +
> +/**
>    * block_job_sleep_ns:
>    * @job: The job that calls the function.
>    * @clock: The clock to sleep on.