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

[Joao Martins]

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  ` 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 
>> ---
>>  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", , ) != 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 
>> */
>>   

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [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  ` 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 
>>> ---
>>>  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", , ) != 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Jim Fehlig]

Joao Martins wrote:
> Introduce support for domainInterfaceStats API call for querying
> network interface statistics. Consequently it also enables the
> use of `virsh domifstat  ` 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 
> ---
>  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", , ) != 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".

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 
> */
>   

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Joao Martins]

Introduce support for domainInterfaceStats API call for querying
network interface statistics. Consequently it also enables the
use of `virsh domifstat  ` 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 
---
 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", , ) != 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;
+}
+
+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 */
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list