| Message ID | 1343127865-16608-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 24.07.2012 13:03, schrieb Paolo Bonzini: > The DeviceNotActive error is not a particularly good match, add > a separate one. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Luiz, what do you think about this one? It seems to contradict the idea of having only few error classes and free form error descriptions. What's the error class that we should really have here? A general QERR_NOT_ACTIVE? Kevin > --- > blockdev.c | 4 ++-- > qapi-schema.json | 5 ++--- > qerror.c | 4 ++++ > qerror.h | 3 +++ > 4 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 3d75015..9c142ee 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1139,7 +1139,7 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) > BlockJob *job = find_block_job(device); > > if (!job) { > - error_set(errp, QERR_DEVICE_NOT_ACTIVE, device); > + error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device); > return; > } > > @@ -1151,7 +1151,7 @@ void qmp_block_job_cancel(const char *device, Error **errp) > BlockJob *job = find_block_job(device); > > if (!job) { > - error_set(errp, QERR_DEVICE_NOT_ACTIVE, device); > + error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device); > return; > } > > diff --git a/qapi-schema.json b/qapi-schema.json > index 000eb83..040981e 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1657,7 +1657,7 @@ > # Returns: Nothing on success > # If the job type does not support throttling, NotSupported > # If the speed value is invalid, InvalidParameter > -# If no background operation is active on this device, DeviceNotActive > +# If no background operation is active on this device, BlockJobNotActive > # > # Since: 1.1 > ## > @@ -1685,8 +1685,7 @@ > # @device: the device name > # > # Returns: Nothing on success > -# If no background operation is active on this device, DeviceNotActive > -# If cancellation already in progress, DeviceInUse > +# If no background operation is active on this device, BlockJobNotActive > # > # Since: 1.1 > ## > diff --git a/qerror.c b/qerror.c > index 92c4eff..bc672a5 100644 > --- a/qerror.c > +++ b/qerror.c > @@ -60,6 +60,10 @@ static const QErrorStringTable qerror_table[] = { > .desc = "Base '%(base)' not found", > }, > { > + .error_fmt = QERR_BLOCK_JOB_NOT_ACTIVE, > + .desc = "No active block job on device '%(name)'", > + }, > + { > .error_fmt = QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > .desc = "Block format '%(format)' used by device '%(name)' does not support feature '%(feature)'", > }, > diff --git a/qerror.h b/qerror.h > index b4c8758..7cf7d22 100644 > --- a/qerror.h > +++ b/qerror.h > @@ -64,6 +64,9 @@ QError *qobject_to_qerror(const QObject *obj); > #define QERR_BASE_NOT_FOUND \ > "{ 'class': 'BaseNotFound', 'data': { 'base': %s } }" > > +#define QERR_BLOCK_JOB_NOT_ACTIVE \ > + "{ 'class': 'BlockJobNotActive', 'data': { 'name': %s } }" > + > #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \ > "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }" > >
Il 26/07/2012 17:26, Kevin Wolf ha scritto: >> The DeviceNotActive error is not a particularly good match, add >> > a separate one. >> > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Luiz, what do you think about this one? It seems to contradict the idea > of having only few error classes and free form error descriptions. I agree, but that's what we have to live with for now... > What's the error class that we should really have here? A general > QERR_NOT_ACTIVE? See my proposal here: http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00061.html (totally ignored ;)). This would be QERR_INVALID_STATE (quoting from that message: "InvalidStateError is generally caused by the interaction with other commands, could be fixed by sending some commands and retrying"). Paolo
On Thu, 26 Jul 2012 17:41:01 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/07/2012 17:26, Kevin Wolf ha scritto: > >> The DeviceNotActive error is not a particularly good match, add > >> > a separate one. > >> > > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Luiz, what do you think about this one? It seems to contradict the idea > > of having only few error classes and free form error descriptions. > > I agree, but that's what we have to live with for now... Why don't you add QERR_INVALID_STATE then? > > > What's the error class that we should really have here? A general > > QERR_NOT_ACTIVE? > > See my proposal here: > http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00061.html > (totally ignored ;)). > > This would be QERR_INVALID_STATE (quoting from that message: > "InvalidStateError is generally caused by the interaction with other > commands, could be fixed by sending some commands and retrying"). > > Paolo >
Il 26/07/2012 18:49, Luiz Capitulino ha scritto: > On Thu, 26 Jul 2012 17:41:01 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 26/07/2012 17:26, Kevin Wolf ha scritto: >>>> The DeviceNotActive error is not a particularly good match, add >>>>> a separate one. >>>>> >>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> Luiz, what do you think about this one? It seems to contradict the idea >>> of having only few error classes and free form error descriptions. >> >> I agree, but that's what we have to live with for now... > > Why don't you add QERR_INVALID_STATE then? Because I do want a meaningful error message. Paolo >> >>> What's the error class that we should really have here? A general >>> QERR_NOT_ACTIVE? >> >> See my proposal here: >> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00061.html >> (totally ignored ;)). >> >> This would be QERR_INVALID_STATE (quoting from that message: >> "InvalidStateError is generally caused by the interaction with other >> commands, could be fixed by sending some commands and retrying"). >> >> Paolo >> >
On Thu, 26 Jul 2012 18:59:30 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/07/2012 18:49, Luiz Capitulino ha scritto: > > On Thu, 26 Jul 2012 17:41:01 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 26/07/2012 17:26, Kevin Wolf ha scritto: > >>>> The DeviceNotActive error is not a particularly good match, add > >>>>> a separate one. > >>>>> > >>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >>> Luiz, what do you think about this one? It seems to contradict the idea > >>> of having only few error classes and free form error descriptions. > >> > >> I agree, but that's what we have to live with for now... > > > > Why don't you add QERR_INVALID_STATE then? > > Because I do want a meaningful error message. It's fine with me adding QERR_BLOCK_JOB_NOT_ACTIVE then, as you've said, it's what we have today.
diff --git a/blockdev.c b/blockdev.c index 3d75015..9c142ee 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1139,7 +1139,7 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) BlockJob *job = find_block_job(device); if (!job) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, device); + error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device); return; } @@ -1151,7 +1151,7 @@ void qmp_block_job_cancel(const char *device, Error **errp) BlockJob *job = find_block_job(device); if (!job) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, device); + error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device); return; } diff --git a/qapi-schema.json b/qapi-schema.json index 000eb83..040981e 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1657,7 +1657,7 @@ # Returns: Nothing on success # If the job type does not support throttling, NotSupported # If the speed value is invalid, InvalidParameter -# If no background operation is active on this device, DeviceNotActive +# If no background operation is active on this device, BlockJobNotActive # # Since: 1.1 ## @@ -1685,8 +1685,7 @@ # @device: the device name # # Returns: Nothing on success -# If no background operation is active on this device, DeviceNotActive -# If cancellation already in progress, DeviceInUse +# If no background operation is active on this device, BlockJobNotActive # # Since: 1.1 ## diff --git a/qerror.c b/qerror.c index 92c4eff..bc672a5 100644 --- a/qerror.c +++ b/qerror.c @@ -60,6 +60,10 @@ static const QErrorStringTable qerror_table[] = { .desc = "Base '%(base)' not found", }, { + .error_fmt = QERR_BLOCK_JOB_NOT_ACTIVE, + .desc = "No active block job on device '%(name)'", + }, + { .error_fmt = QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, .desc = "Block format '%(format)' used by device '%(name)' does not support feature '%(feature)'", }, diff --git a/qerror.h b/qerror.h index b4c8758..7cf7d22 100644 --- a/qerror.h +++ b/qerror.h @@ -64,6 +64,9 @@ QError *qobject_to_qerror(const QObject *obj); #define QERR_BASE_NOT_FOUND \ "{ 'class': 'BaseNotFound', 'data': { 'base': %s } }" +#define QERR_BLOCK_JOB_NOT_ACTIVE \ + "{ 'class': 'BlockJobNotActive', 'data': { 'name': %s } }" + #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \ "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
The DeviceNotActive error is not a particularly good match, add a separate one. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- blockdev.c | 4 ++-- qapi-schema.json | 5 ++--- qerror.c | 4 ++++ qerror.h | 3 +++ 4 files changed, 11 insertions(+), 5 deletions(-)