Re: [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1

[Jeff Cody]

On Thu, Oct 13, 2016 at 06:57:01PM -0400, John Snow wrote:
> To make it a little more obvious which functions are intended to be
> public interface and which are intended to be for use only by jobs
> themselves, split the interface into "public" and "private" files.
> 
> Convert blockjobs (e.g. block/backup) to using the private interface.
> Leave blockdev and others on the public interface.
> 
> There are remaining uses of private state by qemu-img, and several
> cases in blockdev.c and block/io.c where we grab job->blk for the
> purposes of acquiring an AIOContext.
> 
> These will be corrected in future patches.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c               |   2 +-
>  block/commit.c               |   2 +-
>  block/mirror.c               |   2 +-
>  block/stream.c               |   2 +-
>  blockjob.c                   |   2 +-
>  include/block/block.h        |   3 +-
>  include/block/blockjob.h     | 205 +-------------------------------------
>  include/block/blockjob_int.h | 232 
> +++++++++++++++++++++++++++++++++++++++++++
>  tests/test-blockjob-txn.c    |   2 +-
>  tests/test-blockjob.c        |   2 +-
>  10 files changed, 244 insertions(+), 210 deletions(-)
>  create mode 100644 include/block/blockjob_int.h
> 
> diff --git a/block/backup.c b/block/backup.c
> index 6a60ca8..6d12100 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -16,7 +16,7 @@
>  #include "trace.h"
>  #include "block/block.h"
>  #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "block/block_backup.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> diff --git a/block/commit.c b/block/commit.c
> index 475a375..d555600 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -15,7 +15,7 @@
>  #include "qemu/osdep.h"
>  #include "trace.h"
>  #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
> diff --git a/block/mirror.c b/block/mirror.c
> index 4374fb4..c81b5e0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -13,7 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "trace.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/error.h"
> diff --git a/block/stream.c b/block/stream.c
> index 7d6877d..906f7f3 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -14,7 +14,7 @@
>  #include "qemu/osdep.h"
>  #include "trace.h"
>  #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
> diff --git a/blockjob.c b/blockjob.c
> index d118a1f..e6f0d97 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -27,7 +27,7 @@
>  #include "qemu-common.h"
>  #include "trace.h"
>  #include "block/block.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/qmp/qerror.h"
> diff --git a/include/block/block.h b/include/block/block.h
> index 107c603..89b5feb 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -7,16 +7,15 @@
>  #include "qemu/coroutine.h"
>  #include "block/accounting.h"
>  #include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qapi-types.h"
>  #include "qemu/hbitmap.h"
>  
>  /* block.c */
>  typedef struct BlockDriver BlockDriver;
> -typedef struct BlockJob BlockJob;
>  typedef struct BdrvChild BdrvChild;
>  typedef struct BdrvChildRole BdrvChildRole;
> -typedef struct BlockJobTxn BlockJobTxn;
>  
>  typedef struct BlockDriverInfo {
>      /* in bytes, 0 if irrelevant */
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 5b61140..bfc8233 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -28,78 +28,15 @@
>  
>  #include "block/block.h"
>  
> -/**
> - * BlockJobDriver:
> - *
> - * A class type for block job driver.
> - */
> -typedef struct BlockJobDriver {
> -    /** Derived BlockJob struct size */
> -    size_t instance_size;
> -
> -    /** String describing the operation, part of query-block-jobs QMP API */
> -    BlockJobType job_type;
> -
> -    /** Optional callback for job types that support setting a speed limit */
> -    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> -
> -    /** Optional callback for job types that need to forward I/O status 
> reset */
> -    void (*iostatus_reset)(BlockJob *job);
> -
> -    /**
> -     * Optional callback for job types whose completion must be triggered
> -     * 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.
> -     *
> -     * All jobs will complete with a call to either .commit() or .abort() but
> -     * never both.
> -     */
> -    void (*commit)(BlockJob *job);
> -
> -    /**
> -     * 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.
> -     *
> -     * All jobs will complete with a call to either .commit() or .abort() but
> -     * never both.
> -     */
> -    void (*abort)(BlockJob *job);
> -
> -    /**
> -     * If the callback is not NULL, it will be invoked when the job 
> transitions
> -     * into the paused state.  Paused jobs must not perform any asynchronous
> -     * I/O or event loop activity.  This callback is used to quiesce jobs.
> -     */
> -    void coroutine_fn (*pause)(BlockJob *job);
> -
> -    /**
> -     * If the callback is not NULL, it will be invoked when the job 
> transitions
> -     * out of the paused state.  Any asynchronous I/O or event loop activity
> -     * should be restarted from this callback.
> -     */
> -    void coroutine_fn (*resume)(BlockJob *job);
> -
> -    /*
> -     * If the callback is not NULL, it will be invoked before the job is
> -     * resumed in a new AioContext.  This is the place to move any resources
> -     * besides job->blk to the new AioContext.
> -     */
> -    void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> -} BlockJobDriver;
> +typedef struct BlockJobDriver BlockJobDriver;
> +typedef struct BlockJobTxn BlockJobTxn;
>  
>  /**
>   * BlockJob:
>   *
>   * Long-running operation on a BlockDriverState.
>   */
> -struct BlockJob {
> +typedef struct BlockJob {
>      /** The job type, including the job vtable.  */
>      const BlockJobDriver *driver;
>  
> @@ -198,7 +135,7 @@ struct BlockJob {
>      /** Non-NULL if this job is part of a transaction */
>      BlockJobTxn *txn;
>      QLIST_ENTRY(BlockJob) txn_list;
> -};
> +} BlockJob;
>  
>  typedef enum BlockJobCreateFlags {
>      BLOCK_JOB_DEFAULT = 0x00,
> @@ -227,76 +164,6 @@ BlockJob *block_job_next(BlockJob *job);
>  BlockJob *block_job_get(const char *id);
>  
>  /**
> - * block_job_create:
> - * @job_id: The id of the newly-created job, or %NULL to have one
> - * generated automatically.
> - * @job_type: The class object for the newly-created job.
> - * @bs: The block
> - * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> - * @errp: Error object.
> - *
> - * Create a new long-running block device job and return it.  The job
> - * will call @cb asynchronously when the job completes.  Note that
> - * @bs may have been closed at the time the @cb it is called.  If
> - * this is the case, the job may be reported as either cancelled or
> - * completed.
> - *
> - * This function is not part of the public job interface; it should be
> - * called from a wrapper that is specific to the job type.
> - */
> -void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> -                       BlockDriverState *bs, int64_t speed, int flags,
> -                       BlockCompletionFunc *cb, void *opaque, Error **errp);
> -
> -/**
> - * block_job_sleep_ns:
> - * @job: The job that calls the function.
> - * @clock: The clock to sleep on.
> - * @ns: How many nanoseconds to stop for.
> - *
> - * Put the job to sleep (assuming that it wasn't canceled) for @ns
> - * nanoseconds.  Canceling the job will interrupt the wait immediately.
> - */
> -void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> -
> -/**
> - * block_job_yield:
> - * @job: The job that calls the function.
> - *
> - * Yield the block job coroutine.
> - */
> -void block_job_yield(BlockJob *job);
> -
> -/**
> - * block_job_ref:
> - * @bs: The block device.
> - *
> - * Grab a reference to the block job. Should be paired with block_job_unref.
> - */
> -void block_job_ref(BlockJob *job);
> -
> -/**
> - * block_job_unref:
> - * @bs: The block device.
> - *
> - * Release reference to the block job and release resources if it is the last
> - * reference.
> - */
> -void block_job_unref(BlockJob *job);
> -
> -/**
> - * block_job_completed:
> - * @job: The job being completed.
> - * @ret: The status code.
> - *
> - * Call the completion function that was registered at creation time, and
> - * free @job.
> - */
> -void block_job_completed(BlockJob *job, int ret);
> -
> -/**
>   * block_job_set_speed:
>   * @job: The job to set the speed for.
>   * @speed: The new value
> @@ -325,14 +192,6 @@ void block_job_cancel(BlockJob *job);
>  void block_job_complete(BlockJob *job, Error **errp);
>  
>  /**
> - * block_job_is_cancelled:
> - * @job: The job being queried.
> - *
> - * Returns whether the job is scheduled for cancellation.
> - */
> -bool block_job_is_cancelled(BlockJob *job);
> -
> -/**
>   * block_job_query:
>   * @job: The job to get information about.
>   *
> @@ -341,15 +200,6 @@ bool block_job_is_cancelled(BlockJob *job);
>  BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>  
>  /**
> - * block_job_pause_point:
> - * @job: The job that is ready to pause.
> - *
> - * Pause now if block_job_pause() has been called.  Block jobs that perform
> - * lots of I/O must call this between requests so that the job can be paused.
> - */
> -void coroutine_fn block_job_pause_point(BlockJob *job);
> -
> -/**
>   * block_job_pause:
>   * @job: The job to be paused.
>   *
> @@ -392,22 +242,6 @@ void block_job_resume(BlockJob *job);
>  void block_job_user_resume(BlockJob *job);
>  
>  /**
> - * block_job_enter:
> - * @job: The job to enter.
> - *
> - * Continue the specified job by entering the coroutine.
> - */
> -void block_job_enter(BlockJob *job);
> -
> -/**
> - * block_job_ready:
> - * @job: The job which is now ready to complete.
> - *
> - * Send a BLOCK_JOB_READY event for the specified job.
> - */
> -void block_job_event_ready(BlockJob *job);
> -
> -/**
>   * block_job_cancel_sync:
>   * @job: The job to be canceled.
>   *
> @@ -453,37 +287,6 @@ int block_job_complete_sync(BlockJob *job, Error **errp);
>  void block_job_iostatus_reset(BlockJob *job);
>  
>  /**
> - * block_job_error_action:
> - * @job: The job to signal an error for.
> - * @on_err: The error action setting.
> - * @is_read: Whether the operation was a read.
> - * @error: The error that was reported.
> - *
> - * Report an I/O error for a block job and possibly stop the VM.  Return the
> - * action that was selected based on @on_err and @error.
> - */
> -BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError 
> on_err,
> -                                        int is_read, int error);
> -
> -typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> -
> -/**
> - * block_job_defer_to_main_loop:
> - * @job: The job
> - * @fn: The function to run in the main loop
> - * @opaque: The opaque value that is passed to @fn
> - *
> - * Execute a given function in the main loop with the BlockDriverState
> - * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
> - * anything that uses bdrv_drain_all() in the main loop.
> - *
> - * The @job AioContext is held while @fn executes.
> - */
> -void block_job_defer_to_main_loop(BlockJob *job,
> -                                  BlockJobDeferToMainLoopFn *fn,
> -                                  void *opaque);
> -
> -/**
>   * block_job_txn_new:
>   *
>   * Allocate and return a new block job transaction.  Jobs can be added to the
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> new file mode 100644
> index 0000000..8eced19
> --- /dev/null
> +++ b/include/block/blockjob_int.h
> @@ -0,0 +1,232 @@
> +/*
> + * Declarations for long-running block device operations
> + *
> + * Copyright (c) 2011 IBM Corp.
> + * Copyright (c) 2012 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef BLOCKJOB_INT_H
> +#define BLOCKJOB_INT_H
> +
> +#include "block/blockjob.h"
> +#include "block/block.h"
> +
> +/**
> + * BlockJobDriver:
> + *
> + * A class type for block job driver.
> + */
> +struct BlockJobDriver {
> +    /** Derived BlockJob struct size */
> +    size_t instance_size;
> +
> +    /** String describing the operation, part of query-block-jobs QMP API */
> +    BlockJobType job_type;
> +
> +    /** Optional callback for job types that support setting a speed limit */
> +    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> +
> +    /** Optional callback for job types that need to forward I/O status 
> reset */
> +    void (*iostatus_reset)(BlockJob *job);
> +
> +    /**
> +     * Optional callback for job types whose completion must be triggered
> +     * 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.
> +     *
> +     * All jobs will complete with a call to either .commit() or .abort() but
> +     * never both.
> +     */
> +    void (*commit)(BlockJob *job);
> +
> +    /**
> +     * 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.
> +     *
> +     * All jobs will complete with a call to either .commit() or .abort() but
> +     * never both.
> +     */
> +    void (*abort)(BlockJob *job);
> +
> +    /**
> +     * If the callback is not NULL, it will be invoked when the job 
> transitions
> +     * into the paused state.  Paused jobs must not perform any asynchronous
> +     * I/O or event loop activity.  This callback is used to quiesce jobs.
> +     */
> +    void coroutine_fn (*pause)(BlockJob *job);
> +
> +    /**
> +     * If the callback is not NULL, it will be invoked when the job 
> transitions
> +     * out of the paused state.  Any asynchronous I/O or event loop activity
> +     * should be restarted from this callback.
> +     */
> +    void coroutine_fn (*resume)(BlockJob *job);
> +
> +    /*
> +     * If the callback is not NULL, it will be invoked before the job is
> +     * resumed in a new AioContext.  This is the place to move any resources
> +     * besides job->blk to the new AioContext.
> +     */
> +    void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> +};
> +
> +/**
> + * block_job_create:
> + * @job_id: The id of the newly-created job, or %NULL to have one
> + * generated automatically.
> + * @job_type: The class object for the newly-created job.
> + * @bs: The block
> + * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @cb: Completion function for the job.
> + * @opaque: Opaque pointer value passed to @cb.
> + * @errp: Error object.
> + *
> + * Create a new long-running block device job and return it.  The job
> + * will call @cb asynchronously when the job completes.  Note that
> + * @bs may have been closed at the time the @cb it is called.  If
> + * this is the case, the job may be reported as either cancelled or
> + * completed.
> + *
> + * This function is not part of the public job interface; it should be
> + * called from a wrapper that is specific to the job type.
> + */
> +void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> +                       BlockDriverState *bs, int64_t speed, int flags,
> +                       BlockCompletionFunc *cb, void *opaque, Error **errp);
> +
> +/**
> + * block_job_sleep_ns:
> + * @job: The job that calls the function.
> + * @clock: The clock to sleep on.
> + * @ns: How many nanoseconds to stop for.
> + *
> + * Put the job to sleep (assuming that it wasn't canceled) for @ns
> + * nanoseconds.  Canceling the job will interrupt the wait immediately.
> + */
> +void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> +
> +/**
> + * block_job_yield:
> + * @job: The job that calls the function.
> + *
> + * Yield the block job coroutine.
> + */
> +void block_job_yield(BlockJob *job);
> +
> +/**
> + * block_job_ref:
> + * @bs: The block device.
> + *
> + * Grab a reference to the block job. Should be paired with block_job_unref.
> + */
> +void block_job_ref(BlockJob *job);
> +
> +/**
> + * block_job_unref:
> + * @bs: The block device.
> + *
> + * Release reference to the block job and release resources if it is the last
> + * reference.
> + */
> +void block_job_unref(BlockJob *job);
> +
> +/**
> + * block_job_completed:
> + * @job: The job being completed.
> + * @ret: The status code.
> + *
> + * Call the completion function that was registered at creation time, and
> + * free @job.
> + */
> +void block_job_completed(BlockJob *job, int ret);
> +
> +/**
> + * block_job_is_cancelled:
> + * @job: The job being queried.
> + *
> + * Returns whether the job is scheduled for cancellation.
> + */
> +bool block_job_is_cancelled(BlockJob *job);
> +
> +/**
> + * block_job_pause_point:
> + * @job: The job that is ready to pause.
> + *
> + * Pause now if block_job_pause() has been called.  Block jobs that perform
> + * lots of I/O must call this between requests so that the job can be paused.
> + */
> +void coroutine_fn block_job_pause_point(BlockJob *job);
> +
> +/**
> + * block_job_enter:
> + * @job: The job to enter.
> + *
> + * Continue the specified job by entering the coroutine.
> + */
> +void block_job_enter(BlockJob *job);
> +
> +/**
> + * block_job_ready:
> + * @job: The job which is now ready to complete.
> + *
> + * Send a BLOCK_JOB_READY event for the specified job.
> + */
> +void block_job_event_ready(BlockJob *job);
> +
> +/**
> + * block_job_error_action:
> + * @job: The job to signal an error for.
> + * @on_err: The error action setting.
> + * @is_read: Whether the operation was a read.
> + * @error: The error that was reported.
> + *
> + * Report an I/O error for a block job and possibly stop the VM.  Return the
> + * action that was selected based on @on_err and @error.
> + */
> +BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError 
> on_err,
> +                                        int is_read, int error);
> +
> +typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> +
> +/**
> + * block_job_defer_to_main_loop:
> + * @job: The job
> + * @fn: The function to run in the main loop
> + * @opaque: The opaque value that is passed to @fn
> + *
> + * Execute a given function in the main loop with the BlockDriverState
> + * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
> + * anything that uses bdrv_drain_all() in the main loop.
> + *
> + * The @job AioContext is held while @fn executes.
> + */
> +void block_job_defer_to_main_loop(BlockJob *job,
> +                                  BlockJobDeferToMainLoopFn *fn,
> +                                  void *opaque);
> +
> +#endif
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index b79e0c6..f9afc3b 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -13,7 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "sysemu/block-backend.h"
>  
>  typedef struct {
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 18bf850..60b78a3 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -13,7 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "sysemu/block-backend.h"
>  
>  static const BlockJobDriver test_block_job_driver = {
> -- 
> 2.7.4
> 

Reviewed-by: Jeff Cody <address@hidden>