Patchwork [10/10] monitor: do_info_balloon(): use QError

login
register
mail settings
Submitter Luiz Capitulino
Date Nov. 17, 2009, 7:43 p.m.
Message ID <1258487037-24950-11-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/38694/
State New
Headers show

Comments

Luiz Capitulino - Nov. 17, 2009, 7:43 p.m.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Markus Armbruster - Nov. 18, 2009, 3:17 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 74abef9..e42434f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1722,10 +1722,11 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
>  
>      actual = qemu_balloon_status();
>      if (kvm_enabled() && !kvm_has_sync_mmu())
> -        monitor_printf(mon, "Using KVM without synchronous MMU, "
> -                       "ballooning disabled\n");
> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> +                      "Using KVM without synchronous MMU, ballooning disabled");
>      else if (actual == 0)
> -        monitor_printf(mon, "Ballooning not activated in VM\n");
> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> +                       "Ballooning not activated in VM");
>      else
>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
>  }

In PATCH 7/10:

+#define QERR_SERVICE_UNAVAILABLE \
+        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
+

and

+    {
+        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
+        .desc        = "%(reason)",
+    },

How to do a ServiceUnavailable error with a description that is not a
compile time literal?  Add another macro and error table entry for that?
Anthony Liguori - Nov. 18, 2009, 3:58 p.m.
Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>   
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  monitor.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 74abef9..e42434f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1722,10 +1722,11 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
>>  
>>      actual = qemu_balloon_status();
>>      if (kvm_enabled() && !kvm_has_sync_mmu())
>> -        monitor_printf(mon, "Using KVM without synchronous MMU, "
>> -                       "ballooning disabled\n");
>> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
>> +                      "Using KVM without synchronous MMU, ballooning disabled");
>>      else if (actual == 0)
>> -        monitor_printf(mon, "Ballooning not activated in VM\n");
>> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
>> +                       "Ballooning not activated in VM");
>>      else
>>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
>>  }
>>     
>
> In PATCH 7/10:
>
> +#define QERR_SERVICE_UNAVAILABLE \
> +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> +
>
> and
>
> +    {
> +        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
> +        .desc        = "%(reason)",
> +    },
>
> How to do a ServiceUnavailable error with a description that is not a
> compile time literal?  Add another macro and error table entry for that?
>   

An error that just contains reason is a good indication that the error 
is not the right level of abstraction.

There are two error conditions here.  One is that kvm is in use, but 
it's missing a capability and therefore we have to disable a feature.   
The second error is that the guest did not activate a device.

So the first error should be something like

#define QERR_KVM_MISSING_CAP \
              "{'class': 'KVMMissingCap', 'data' : { 'capability': %s, 
'feature': %s' }"

Where capability is the string form of the missing cap and feature 
describes the feature being disabled because of the missing cap.

The error format would be "Using KVM without %{capability}, %{feature} 
unavailable".  It would get raised with

qemu_error_new(QERR_KVM_MISSING_CAP, kvm_get_cap_desc(KVM_CAP_SYNC_MMU), 
"balloon");

The error text is slightly modified, but that's okay.

Likewise, the second error should be something like

#define QERR_DEVICE_NOT_ACTIVE \
          "{'class': 'DeviceNotActive', 'data' : { 'device': %s }"

qemu_error_new(QERR_DEVICE_NOT_ACTIVE, "balloon");

With a description of "The %{device} device has not been activated by 
the guest"

If I was writing a UI, I would respond very differently to these two 
errors.  For the first error, I would grey out the balloon feature 
because it's not possible to use ballooning with this very of KVM.  For 
the second error, I would prompt the user when they tried to do 
ballooning to make sure the driver was loaded in the guest.

This behavior can not be achieved the ServiceNotAvailable.
Luiz Capitulino - Nov. 18, 2009, 6:10 p.m.
On Wed, 18 Nov 2009 09:58:30 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> Markus Armbruster wrote:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >   
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >>  monitor.c |    7 ++++---
> >>  1 files changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 74abef9..e42434f 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -1722,10 +1722,11 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
> >>  
> >>      actual = qemu_balloon_status();
> >>      if (kvm_enabled() && !kvm_has_sync_mmu())
> >> -        monitor_printf(mon, "Using KVM without synchronous MMU, "
> >> -                       "ballooning disabled\n");
> >> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> >> +                      "Using KVM without synchronous MMU, ballooning disabled");
> >>      else if (actual == 0)
> >> -        monitor_printf(mon, "Ballooning not activated in VM\n");
> >> +        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
> >> +                       "Ballooning not activated in VM");
> >>      else
> >>          *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
> >>  }
> >>     
> >
> > In PATCH 7/10:
> >
> > +#define QERR_SERVICE_UNAVAILABLE \
> > +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> > +
> >
> > and
> >
> > +    {
> > +        .error_fmt   = QERR_SERVICE_UNAVAILABLE,
> > +        .desc        = "%(reason)",
> > +    },
> >
> > How to do a ServiceUnavailable error with a description that is not a
> > compile time literal?  Add another macro and error table entry for that?
> >   
> 
> An error that just contains reason is a good indication that the error 
> is not the right level of abstraction.
> 
> There are two error conditions here.  One is that kvm is in use, but 
> it's missing a capability and therefore we have to disable a feature.   
> The second error is that the guest did not activate a device.

 Thanks for the detailed explanation, I've included the new errors.

Patch

diff --git a/monitor.c b/monitor.c
index 74abef9..e42434f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1722,10 +1722,11 @@  static void do_info_balloon(Monitor *mon, QObject **ret_data)
 
     actual = qemu_balloon_status();
     if (kvm_enabled() && !kvm_has_sync_mmu())
-        monitor_printf(mon, "Using KVM without synchronous MMU, "
-                       "ballooning disabled\n");
+        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
+                      "Using KVM without synchronous MMU, ballooning disabled");
     else if (actual == 0)
-        monitor_printf(mon, "Ballooning not activated in VM\n");
+        qemu_error_new(QERR_SERVICE_UNAVAILABLE,
+                       "Ballooning not activated in VM");
     else
         *ret_data = QOBJECT(qint_from_int((int)(actual >> 20)));
 }