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

On Wed, Apr 25, 2018 at 12:20:30PM +0200, Katerina Koukiou wrote: > Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com&gt; > --- > 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