[libvirt] [dbus PATCH 15/15] Implement GetCPUStats method for Domain Interface

[Katerina Koukiou]

On Wed, 2018-04-25 at 18:33 +0200, Pavel Hrdina wrote:
> On Wed, Apr 25, 2018 at 12:20:30PM +0200, Katerina Koukiou wrote:
> > Signed-off-by: Katerina Koukiou <kkoukiou at redhat.com>
> > ---
> >  data/org.libvirt.Domain.xml |  8 ++++++
> >  src/domain.c                | 68
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 76 insertions(+)
> > 
> > diff --git a/data/org.libvirt.Domain.xml
> > b/data/org.libvirt.Domain.xml
> > index 8d027dd..9292a91 100644
> > --- a/data/org.libvirt.Domain.xml
> > +++ b/data/org.libvirt.Domain.xml
> > @@ -191,6 +191,14 @@
> >        <arg name="flags" type="u" direction="in"/>
> >        <arg name="controlInfo" type="(sst)" direction="out"/>
> >      </method>
> > +    <method name="GetCPUStats">
> > +      <annotation name="org.gtk.GDBus.DocString"
> > +        value="See https://libvirt.org/html/libvirt-libvirt-domain
> > .html#virDomainGetCPUStats
> > +               @total should be set to get total stats instead of
> > per-cpu stats."/>
> 
> I would rephrase it:
> 
> "If @total is True, it returns total cpu stats, otherwise it returns
> per-cpu stats."
> 
> > +      <arg name="total" type="b" direction="in"/>
> > +      <arg name="flags" type="u" direction="in"/>
> > +      <arg name="stats" type="a(sa{sv})" direction="out"/>
> > +    </method>
> >      <method name="GetDiskErrors">
> >        <annotation name="org.gtk.GDBus.DocString"
> >          value="See https://libvirt.org/html/libvirt-libvirt-domain
> > .html#virDomainGetDiskErrors"/>;
> > diff --git a/src/domain.c b/src/domain.c
> > index 608f1d1..1c09cc9 100644
> > --- a/src/domain.c
> > +++ b/src/domain.c
> > @@ -3,6 +3,8 @@
> >  
> >  #include <libvirt/libvirt.h>
> >  
> > +#define VIRT_DBUS_DOMAIN_MAX_CPUS 128
> > +
> >  VIRT_DBUS_ENUM_DECL(virtDBusDomainBlockJob)
> >  VIRT_DBUS_ENUM_IMPL(virtDBusDomainBlockJob,
> >                      VIR_DOMAIN_BLOCK_JOB_TYPE_LAST,
> > @@ -1041,6 +1043,71 @@ virtDBusDomainGetControlInfo(GVariant
> > *inArgs,
> >                               errorReasonStr, controlInfo-
> > >stateTime);
> >  }
> >  
> > +static void
> > +virtDBusDomainGetCPUStats(GVariant *inArgs,
> > +                          GUnixFDList *inFDs G_GNUC_UNUSED,
> > +                          const gchar *objectPath,
> > +                          gpointer userData,
> > +                          GVariant **outArgs,
> > +                          GUnixFDList **outFDs G_GNUC_UNUSED,
> > +                          GError **error)
> > +{
> > +    virtDBusConnect *connect = userData;
> > +    g_autoptr(virDomain) domain = NULL;
> > +    g_auto(virtDBusUtilTypedParams) params = { 0 };
> > +    gint startCPU = 0;
> > +    guint ncpus = VIRT_DBUS_DOMAIN_MAX_CPUS;
> > +    gboolean total;
> > +    guint flags;
> > +    GVariant *gret;
> > +    GVariantBuilder builder;
> > +    gint ret;
> > +
> > +    g_variant_get(inArgs, "(bu)", &total, &flags);
> > +
> > +    domain = virtDBusDomainGetVirDomain(connect, objectPath,
> > error);
> > +    if (!domain)
> > +        return;
> 
> So this libvirt API has really stupid design and it's super
> complicated,
> I'm considering dropping it completely since the only benefit of this
> API is that you can get per host cpu statistic for specific domain.
> 
> However, if we decide to keep it, it has to be rewritten.  Currently
> the
> code would report only first 128 host CPUs which is not correct.
> 
> You can look at libvirt-python implementation [1] of this API to get
> the
> idea of how it needs to be done, basically if there is more than 128
> host CPUs, the virDomainGetCPUStats() needs to be called multiple
> times
> to get all statistics.
> 
> [1] libvirt-override.c (libvirt_virDomainGetCPUStats)
> 
> Pavel

Thanks for the heads up. So, I 'll drop it for now and I 'll leave it
for future.

Katerina