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

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

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?

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.

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.