Re: [Qemu-devel] [PATCH 2/4] block: use Error mechanism instead of -errno for block_job_set_speed()

[Paolo Bonzini]

Il 23/04/2012 17:39, Stefan Hajnoczi ha scritto:
> There are at least two different errors that can occur in
> block_job_set_speed(): the job might not support setting speeds or the
> value might be invalid.
> 
> Use the Error mechanism to report the error where it occurs.  This patch
> adds the new BlockJobSpeedInvalid QError.  The error is specific to
> block job speeds so we can add a speed argument to block-stream in the
> future and clearly identify the invalid parameter.
> 
> Cc: Luiz Capitulino <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c          |   17 ++++++++++-------
>  block/stream.c   |    6 +++---
>  block_int.h      |    5 +++--
>  blockdev.c       |    4 +---
>  qapi-schema.json |    1 +
>  qerror.c         |    4 ++++
>  qerror.h         |    3 +++
>  7 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 528b696..7056d8c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4103,18 +4103,21 @@ void block_job_complete(BlockJob *job, int ret)
>      bdrv_set_in_use(bs, 0);
>  }
>  
> -int block_job_set_speed(BlockJob *job, int64_t value)
> +void block_job_set_speed(BlockJob *job, int64_t value, Error **errp)
>  {
> -    int rc;
> +    Error *local_error = NULL;
>  
>      if (!job->job_type->set_speed) {
> -        return -ENOTSUP;
> +        error_set(errp, QERR_NOT_SUPPORTED);
> +        return;
>      }
> -    rc = job->job_type->set_speed(job, value);
> -    if (rc == 0) {
> -        job->speed = value;
> +    job->job_type->set_speed(job, value, &local_error);
> +    if (error_is_set(&local_error)) {
> +        error_propagate(errp, local_error);
> +        return;
>      }
> -    return rc;
> +
> +    job->speed = value;

I don't think this is the right place to add Error handling.  By doing
it at QAPI entry points we can reuse an existing error such as
QERR_INVALID_PARAMETER_VALUE.

If we need to introduce a specific error for each parameter type, we
will end up with N errors per function.

Luiz, what do you think?

Paolo

>  }
>  
>  void block_job_cancel(BlockJob *job)
> diff --git a/block/stream.c b/block/stream.c
> index 0aa7083..f0486a3 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -263,15 +263,15 @@ retry:
>      block_job_complete(&s->common, ret);
>  }
>  
> -static int stream_set_speed(BlockJob *job, int64_t value)
> +static void stream_set_speed(BlockJob *job, int64_t value, Error **errp)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common);
>  
>      if (value < 0) {
> -        return -EINVAL;
> +        error_set(errp, QERR_BLOCK_JOB_SPEED_INVALID, value);
> +        return;
>      }
>      ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
> -    return 0;
>  }
>  
>  static BlockJobType stream_job_type = {
> diff --git a/block_int.h b/block_int.h
> index 1557d5c..6015c27 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -78,7 +78,7 @@ typedef struct BlockJobType {
>      const char *job_type;
>  
>      /** Optional callback for job types that support setting a speed limit */
> -    int (*set_speed)(BlockJob *job, int64_t value);
> +    void (*set_speed)(BlockJob *job, int64_t value, Error **errp);
>  } BlockJobType;
>  
>  /**
> @@ -374,11 +374,12 @@ void block_job_complete(BlockJob *job, int ret);
>   * block_job_set_speed:
>   * @job: The job to set the speed for.
>   * @speed: The new value
> + * @errp: A location to return NotSupported or BlockJobSpeedInvalid.
>   *
>   * Set a rate-limiting parameter for the job; the actual meaning may
>   * vary depending on the job type.
>   */
> -int block_job_set_speed(BlockJob *job, int64_t value);
> +void block_job_set_speed(BlockJob *job, int64_t value, Error **errp);
>  
>  /**
>   * block_job_cancel:
> diff --git a/blockdev.c b/blockdev.c
> index 202c3ae..c484259 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1143,9 +1143,7 @@ void qmp_block_job_set_speed(const char *device, 
> int64_t value, Error **errp)
>          return;
>      }
>  
> -    if (block_job_set_speed(job, value) < 0) {
> -        error_set(errp, QERR_NOT_SUPPORTED);
> -    }
> +    block_job_set_speed(job, value, errp);
>  }
>  
>  void qmp_block_job_cancel(const char *device, Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 6499895..d0d4f693 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1596,6 +1596,7 @@
>  #
>  # Returns: Nothing on success
>  #          If the job type does not support throttling, NotSupported
> +#          If the speed value is invalid, BlockJobSpeedInvalid
>  #          If streaming is not active on this device, DeviceNotActive
>  #
>  # Since: 1.1
> diff --git a/qerror.c b/qerror.c
> index 96fbe71..6aee606 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -64,6 +64,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Block format '%(format)' used by device '%(name)' does 
> not support feature '%(feature)'",
>      },
>      {
> +        .error_fmt = QERR_BLOCK_JOB_SPEED_INVALID,
> +        .desc      = "Block job speed value '%(value)' is invalid",
> +    },
> +    {
>          .error_fmt = QERR_BUS_NO_HOTPLUG,
>          .desc      = "Bus '%(bus)' does not support hotplugging",
>      },
> diff --git a/qerror.h b/qerror.h
> index 5c23c1f..e239ef1 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -67,6 +67,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>      "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 
> 'name': %s, 'feature': %s } }"
>  
> +#define QERR_BLOCK_JOB_SPEED_INVALID \
> +    "{ 'class': 'BlockJobSpeedInvalid', 'data': { 'value': %"PRId64" } }"
> +
>  #define QERR_BUFFER_OVERRUN \
>      "{ 'class': 'BufferOverrun', 'data': {} }"
>