Re: [Xen-devel] [PATCH 3/3] libxl: implement virDomainInterfaceStats

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 11/19/2015 04:45 PM, Joao Martins wrote:
> Introduce support for domainInterfaceStats API call for querying
> network interface statistics. Consequently it also enables the
> use of `virsh domifstat <dom> <interface name>` command plus
> seeing the interfaces names instead of "-" when doing
> `virsh domiflist <dom>`.
>
> After succesful guest creation we fill the network
> interfaces names based on domain, device id and append suffix
> if it's emulated in the following form: vif<domid>.<devid>[-emu].
> The devid is taken from domain config which is accessible in
> libxlDomainStartCallback(). On domain
> cleanup we also clear ifname, in case it was set by libvirt (i.e.
> being prefixed with "vif"). We also skip these two steps in case the name
> of the interface was manually inserted by the adminstrator.
>
> For getting the interface statistics we resort to virNetInterfaceStats
> and let libvirt handle the platform specific nits. Note that the latter
> is not yet supported in FreeBSD.
>
> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> ---
> Changes since v3:
>  - Do not unlock vm if libxlDomainObjEndJob() returns false
>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
>  DomainStart.
>  - Change commit message reflecting the changes on the previous
>  item and mention correct interface names when doing domiflist.
>
> Changes since v2:
>  - Clear ifname if it's autogenerated, since otherwise will persist
>  on successive domain starts. Change commit message reflecting this
>  change.
>
> Changes since v1:
>  - Fill <virDomainNetDef>.ifname after domain start with generated
>  name from libxl  based on domain id and devid returned by libxl.
>  After that path validation don interfaceStats is enterily based
>  on ifname pretty much like the other drivers.
>  - Modify commit message reflecting the changes mentioned in
>  the previous item.
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_domain.c | 27 +++++++++++++++++++++++++++
>  src/libxl/libxl_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index f0746f8..5533677 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -733,6 +733,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>          }
>      }
>  
> +    if ((vm->def->nnets)) {
> +        ssize_t i;
> +
> +        for (i = 0; i < vm->def->nnets; i++) {
> +            virDomainNetDefPtr net = vm->def->nets[i];
> +
> +            if (STRPREFIX(net->ifname, "vif"))
> +                VIR_FREE(net->ifname);
> +        }
> +    }
> +
>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>              VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
> @@ -862,6 +873,8 @@ static void
>  libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>  {
>      virDomainObjPtr vm = for_callback;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    libxl_domain_config *d_config = priv->config;
>      size_t i;
>  
>      virObjectLock(vm);
> @@ -888,6 +901,20 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event 
> *ev, void *for_callback)
>              VIR_FREE(console);
>          }
>      }
> +
> +    for (i = 0; i < vm->def->nnets; i++) {
> +        virDomainNetDefPtr net = vm->def->nets[i];
> +        libxl_device_nic *x_nic = &d_config->nics[i];
> +        const char *suffix =
> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
> +
> +        if (net->ifname)
> +            continue;
> +
> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
> +                        ev->domid, x_nic->devid, suffix) < 0)
> +            continue;
> +    }

Is it possible to get the interfaces with libxl_device_nic_list(), and use those
to generate ifname in the corresponding virDomainNetDef? According to the libxl
docs, it is permitted to call libxl from within the callback.

When I first read this patch, I thought this hunk was in the
libxlDomainInterfaceStats() function below. If that was the case, you can
probably see my concern that vm->def->nets could have grown or shrunk over time
while d_config->nics remained static. Such use of the potentially stale
libxl_domain_config object is why I'm now questioning whether we should add it
to the libxlDomainObjPrivate struct. Sorry for not realizing this when making
the suggestion :-(.

Regards,
Jim

>      virObjectUnlock(vm);
>      libxl_event_free(ctx, ev);
>  }
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d77a0e4..fe9c357 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -58,6 +58,7 @@
>  #include "virhostdev.h"
>  #include "network/bridge_driver.h"
>  #include "locking/domain_lock.h"
> +#include "virstats.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>  
> @@ -4643,6 +4644,50 @@ libxlDomainIsUpdated(virDomainPtr dom)
>  }
>  
>  static int
> +libxlDomainInterfaceStats(virDomainPtr dom,
> +                          const char *path,
> +                          virDomainInterfaceStatsPtr stats)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    ssize_t i;
> +    int ret = -1;
> +
> +    if (!(vm = libxlDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto endjob;
> +    }
> +
> +    /* Check the path is one of the domain's network interfaces. */
> +    for (i = 0; i < vm->def->nnets; i++) {
> +        if (vm->def->nets[i]->ifname &&
> +            STREQ(vm->def->nets[i]->ifname, path)) {
> +            ret = virNetInterfaceStats(path, stats);
> +            break;
> +        }
> +    }
> +
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}
> +
> +static int
>  libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
>                              virDomainObjPtr vm,
>                              virTypedParameterPtr params,
> @@ -5421,6 +5466,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
>      .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>      .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 
> */


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel