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

[Stefan Hajnoczi]

On Mon, Apr 23, 2012 at 7:01 PM, Luiz Capitulino <address@hidden> wrote:
> 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?

"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."

I added the new error to avoid having to change the InvalidParameter
'name' field.  It becomes ugly because we have:
block-job-set-speed <device> <value>
block-stream <device> [<speed>] [<base>]

Notice that the parameter is called 'value' in block-job-set-speed and
'speed' in block-stream.  Therefore we can't just propagate a single
InvalidParameter name='value' error up from block-stream.

This means that QMP commands will need to change the error to use the
correct name :(.  It's doable, but ugly and a bit more complex.

Thoughts?

Stefan