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

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 11/20/2015 07:25 PM, Jim Fehlig wrote:
> 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.
Ah, I didn't know that routine in the libxl API. Then we don't need the config
at all in libxlDomainObjPrivate.

> 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 :-(.
No problem! I think the config there might actually be redundant and with not
much use after domain create as I pointed out in the Patch 1/3 comment.

Regards,
Joao
> 
> 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