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

[Luiz Capitulino]

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': {} }"
> >  
>