Patchwork [02/47] qerror/block: introduce QERR_BLOCK_JOB_NOT_ACTIVE

login
register
mail settings
Submitter Paolo Bonzini
Date July 24, 2012, 11:03 a.m.
Message ID <1343127865-16608-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/172829/
State New
Headers show

Comments

Paolo Bonzini - July 24, 2012, 11:03 a.m.
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(-)
Kevin Wolf - July 26, 2012, 3:26 p.m.
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 } }"
>  
>
Paolo Bonzini - July 26, 2012, 3:41 p.m.
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
Luiz Capitulino - July 26, 2012, 4:49 p.m.
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
>
Paolo Bonzini - July 26, 2012, 4:59 p.m.
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
>>
>
Luiz Capitulino - July 26, 2012, 5:02 p.m.
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.

Patch

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