| Message ID | 1335195589-13285-3-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <lcapitulino@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@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. If we need to introduce a specific error for each parameter type, we will end up with N errors per function. 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': {} }" >
On Mon, 23 Apr 2012 17:47:09 +0200 Paolo Bonzini <pbonzini@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 <lcapitulino@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@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? > > 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': {} }" > > >
On Mon, Apr 23, 2012 at 7:01 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Mon, 23 Apr 2012 17:47:09 +0200 > Paolo Bonzini <pbonzini@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 <lcapitulino@redhat.com> >> > Signed-off-by: Stefan Hajnoczi <stefanha@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
Il 24/04/2012 10:49, Stefan Hajnoczi ha scritto: > "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. Well, we still have time to change block-job-set-speed. Ugly, but doable. Paolo
On 04/24/2012 03:01 AM, Paolo Bonzini wrote: > Il 24/04/2012 10:49, Stefan Hajnoczi ha scritto: >> "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. > > Well, we still have time to change block-job-set-speed. Ugly, but doable. block-job-set-speed <device> <speed> would indeed let you use 'speed' as the parameter name in both calls, and I can just as easily make libvirt target the name 'value' for block_job_set_speed and 'speed' for block-job-set-speed. I'm okay with that idea, if it makes the rest of your patch easier.
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; } 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': {} }"
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 <lcapitulino@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@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(-)