[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] block: use Error mechanism instead of -errn
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] block: use Error mechanism instead of -errno for block_job_set_speed() |
Date: |
Mon, 23 Apr 2012 15:01:28 -0300 |
On Mon, 23 Apr 2012 17:47:09 +0200
Paolo Bonzini <address@hidden> wrote:
> 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.
I think the place were we call error_set() isn't a problem. Actually, the Error
object was specifically designed to be used from qemu depths and be propagated
up.
Now:
> If we need to introduce a specific error for each parameter type, we
> will end up with N errors per function.
I agree with that, QError design induces people to add new errors... Why can't
you use one of the invalid parameter errors we have?
>
> 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': {} }"
> >
>
Re: [Qemu-devel] [PATCH 0/4] block: add optional 'speed' parameter to block-stream, Eric Blake, 2012/04/23
Re: [Qemu-devel] [PATCH 0/4] block: add optional 'speed' parameter to block-stream, Anthony Liguori, 2012/04/23