[libvirt] [PATCH RFC 3/7] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

Joao Martins wrote:
> 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 at 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.
>   

Yeah, I think setting net->ifname after start is a good idea.
libxlConsoleCallback() could be re-purposed to a more generic
libxlDomainStartCallback() or similar, where info provided by libxl is
copied to the virDomainObj.

Regards,
Jim