Re: [Qemu-devel] [PATCH 10/10] monitor: do_info_balloon(): use QError

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

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.

-- 
Regards,

Anthony Liguori