[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: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] block: use Error mechanism instead of -errno for block_job_set_speed() |
Date: |
Tue, 24 Apr 2012 09:49:40 +0100 |
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
- [Qemu-devel] [PATCH 1/4] block: use Error mechanism instead of -errno for block_job_create(), (continued)
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
Re: [Qemu-devel] [PATCH 0/4] block: add optional 'speed' parameter to block-stream, Eric Blake, 2012/04/23