Re: [libvirt] [PATCH RFC 3/7] libxl: implement virDomainInterfaceStats
On 09/23/2015 08:18 PM, Jim Fehlig wrote:
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.
>
> For getting 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(a)oracle.com>
> ---
> src/libxl/libxl_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 43e9e47..dc83083 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
>
> @@ -4640,6 +4641,57 @@ libxlDomainIsUpdated(virDomainPtr dom)
> }
>
> static int
> +libxlDomainInterfaceStats(virDomainPtr dom,
> + const char *path,
> + virDomainInterfaceStatsPtr stats)
> +{
> + libxlDriverPrivatePtr driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> + int domid, devid;
> +
> + 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;
> + }
> +
> + if (sscanf(path, "vif%d.%d", &domid, &devid) != 2) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("invalid path, unknown device"));
> + goto endjob;
> + }
> +
> + if (domid != vm->def->id) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("invalid path, domid doesn't match"));
> + goto endjob;
> + }
>
Should we also ensure the domain has an interface matching devid before
calling virNetInterfaceStats()? I see the qemu driver has such a check,
but virNetInterfaceStats() also reports "Interface not found".
Ultimately I rely on that error and check if the path name is the path
correspondent to a xen interface prefixed by "vif%d.%d". Problem with doing
that
check is that net->ifname is not set unless explicitly. And looking at the qemu
driver: they create the device and fetch the ifname and then add it to the bridge.
Perhaps it is reasonable is if we set the net->ifname after domain create and
then make that check, as you suggest, instead of trying to "guess" the path.
The
way this patch proposes, the user wouldn't be able to get statistics from
anything other than "vif%d.%d[-emu]" which is incorrect.
Regards,
Jim
> +
> + ret = virNetInterfaceStats(path, stats);
> +
> + endjob:
> + if (!libxlDomainObjEndJob(driver, vm)) {
> + virObjectUnlock(vm);
> + vm = NULL;
> + }
> +
> + cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return ret;
> +}
> +
> +static int
> libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
> virDomainObjPtr vm,
> virTypedParameterPtr params,
> @@ -5407,6 +5459,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
> #endif
> .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
> .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */
> .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */
> .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */
> .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>