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

[Anthony Liguori]

Markus Armbruster wrote:
> Luiz Capitulino <address@hidden> writes:
>
>   
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  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