On Mon, Apr 23, 2012 at 7:01 PM, Luiz Capitulino <lcapitul...@redhat.com> wrote: > On Mon, 23 Apr 2012 17:47:09 +0200 > Paolo Bonzini <pbonz...@redhat.com> 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 <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. > > 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