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 <lcapitul...@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@linux.vnet.ibm.com>
> ---
> 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': {} }"
>
