5:45 p.m.
Hey,
As discussed yesterday, this patch series implements virDomainInterfaceStats
but based on "ConsoleCallback" as opposed of doing it in libxlDomainStart.
The series is divided as following: Patch 1 adds up domain config to
libxlDomainObjPrivate, Patch 2 renames console callback to something more
generic and Patch 3 implements virDomainInterfaceStats also taking the
previous review (v3 statistics) comments.
Regards,
Joao
Joao Martins (3):
libxl: add libxl_domain_config to libxlDomainObjPrivate
libxl: Rename libxlConsoleCallback
libxl: implement virDomainInterfaceStats
src/libxl/libxl_domain.c | 65 +++++++++++++++++++++++++++++++++++++++---------
src/libxl/libxl_domain.h | 1 +
src/libxl/libxl_driver.c | 46 ++++++++++++++++++++++++++++++++++
3 files changed, 100 insertions(+), 12 deletions(-)
--
2.1.4
5:45 p.m.
New subject: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
This new field in libxlDomainObjPrivate is named "config"
and is kept while the domain is active. For now, "config"
will be used in libxlDomainStartCallback to set
network interface names based on domid and
libxl_device_nic devid that is set in the config on domain
create.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
src/libxl/libxl_domain.c | 34 ++++++++++++++++++++++++----------
src/libxl/libxl_domain.h | 1 +
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 40dcea1..60ef3a0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -709,6 +709,11 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
vm->def->id = -1;
+ if (priv->config) {
+ libxl_domain_config_dispose(priv->config);
+ VIR_FREE(priv->config);
+ }
+
if (priv->deathW) {
libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
priv->deathW = NULL;
@@ -897,7 +902,7 @@ int
libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
bool start_paused, int restore_fd)
{
- libxl_domain_config d_config;
+ libxl_domain_config *d_config = NULL;
virDomainDefPtr def = NULL;
virObjectEventPtr event = NULL;
libxlSavefileHeader hdr;
@@ -914,7 +919,10 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
libxl_asyncprogress_how aop_console_how;
- libxl_domain_config_init(&d_config);
+ if (VIR_ALLOC(d_config) < 0)
+ return ret;
+
+ libxl_domain_config_init(d_config);
cfg = libxlDriverConfigGet(driver);
/* If there is a managed saved state restore it instead of starting
@@ -960,10 +968,10 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
}
if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
- cfg->ctx, &d_config) < 0)
+ cfg->ctx, d_config) < 0)
goto cleanup;
- if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0)
+ if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, d_config) < 0)
goto cleanup;
if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
@@ -987,19 +995,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
/* Unlock virDomainObj while creating the domain */
virObjectUnlock(vm);
+ priv->config = d_config;
aop_console_how.for_callback = vm;
aop_console_how.callback = libxlConsoleCallback;
if (restore_fd < 0) {
- ret = libxl_domain_create_new(cfg->ctx, &d_config,
+ ret = libxl_domain_create_new(cfg->ctx, d_config,
&domid, NULL, &aop_console_how);
} else {
#ifdef LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS
params.checkpointed_stream = 0;
- ret = libxl_domain_create_restore(cfg->ctx, &d_config, &domid,
+ ret = libxl_domain_create_restore(cfg->ctx, d_config, &domid,
restore_fd, ¶ms, NULL,
&aop_console_how);
#else
- ret = libxl_domain_create_restore(cfg->ctx, &d_config, &domid,
+ ret = libxl_domain_create_restore(cfg->ctx, d_config, &domid,
restore_fd, NULL, &aop_console_how);
#endif
}
@@ -1009,11 +1018,11 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
if (restore_fd < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to create new domain
'%s'"),
- d_config.c_info.name);
+ d_config->c_info.name);
else
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to restore domain
'%s'"),
- d_config.c_info.name);
+ d_config->c_info.name);
goto release_dom;
}
@@ -1061,6 +1070,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
if (event)
libxlDomainEventQueue(driver, event);
+ d_config = NULL;
ret = 0;
goto cleanup;
@@ -1078,7 +1088,11 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState);
cleanup:
- libxl_domain_config_dispose(&d_config);
+ if (d_config) {
+ libxl_domain_config_dispose(d_config);
+ VIR_FREE(d_config);
+ priv->config = NULL;
+ }
VIR_FREE(dom_xml);
VIR_FREE(managed_save_path);
virDomainDefFree(def);
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 44b3e0b..52ee726 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -65,6 +65,7 @@ struct _libxlDomainObjPrivate {
libxl_evgen_domain_death *deathW;
unsigned short migrationPort;
char *lockState;
+ libxl_domain_config *config;
struct libxlDomainJobObj job;
};
--
2.1.4
1:05 p.m.
New subject: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/19/2015 04:45 PM, Joao Martins wrote:
You're not going to be happy with me...
This new field in libxlDomainObjPrivate is named "config"
and is kept while the domain is active.
While this sounded like a good idea when I mentioned it, I'm now worried that
the config will quickly become stale and cause problems if used elsewhere (e.g.
see my yet-to-be-written comment in 3/3). IIUC correctly, libxl_domain_config
is only useful when creating the domain. Subsequently adding/removing devices,
memory, vcpus, etc. would not be reflected in the libxl_domain_config object. I
suppose it would useful (and still valid) in the start callback, but IMO
including it in the libxlDomainPrivate struct fools us into believing it could
be used elsewhere throughout the life of the domain. I now have second doubts
about this. What do you think?
Regards,
Jim
6:40 p.m.
New subject: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/20/2015 07:05 PM, Jim Fehlig wrote:
On 11/19/2015 04:45 PM, Joao Martins wrote:
You're not going to be happy with me...
> This new field in libxlDomainObjPrivate is named "config"
> and is kept while the domain is active.
While this sounded like a good idea when I mentioned it, I'm now worried that
the config will quickly become stale and cause problems if used elsewhere (e.g.
see my yet-to-be-written comment in 3/3). IIUC correctly, libxl_domain_config
is only useful when creating the domain. Subsequently adding/removing devices,
memory, vcpus, etc. would not be reflected in the libxl_domain_config object. I
suppose it would useful (and still valid) in the start callback, but IMO
including it in the libxlDomainPrivate struct fools us into believing it could
be used elsewhere throughout the life of the domain. I now have second doubts
about this. What do you think?
I agree with you, and since there a
libxl_device_nic_list as you suggested, it
would actually be much cleaner and safer compared to libxl_domain_config
alternative (though with a small performance cost). And we would avoid end up
having config just lying there with no additional use (besides StartCallback)
and inconsistent info.
The only thing that the libxlDomainObjPrivate approach is better than
libxl_device_nic_list() would be that we don't need to refetch the devid, since
the nics array has it correctly filled already when console callback is invoked.
Whereas libxl_device_nic_list will refetch the same info (in additiona to all
entries in the backend directory) from xenstore thus adding up overhead. But
given that this is only once and in domain create I think it's not a big deal.
Would you agree then to resend this series without this patch and using
libxl_device_nic_list, as the final approach? Thanks for pointing out this issue!
Cheers,
Joao
Regards,
Jim
9:35 a.m.
New subject: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/20/2015 05:40 PM, Joao Martins wrote:
On 11/20/2015 07:05 PM, Jim Fehlig wrote:
> On 11/19/2015 04:45 PM, Joao Martins wrote:
>
> You're not going to be happy with me...
>
>> This new field in libxlDomainObjPrivate is named "config"
>> and is kept while the domain is active.
> While this sounded like a good idea when I mentioned it, I'm now worried that
> the config will quickly become stale and cause problems if used elsewhere (e.g.
> see my yet-to-be-written comment in 3/3). IIUC correctly, libxl_domain_config
> is only useful when creating the domain. Subsequently adding/removing devices,
> memory, vcpus, etc. would not be reflected in the libxl_domain_config object. I
> suppose it would useful (and still valid) in the start callback, but IMO
> including it in the libxlDomainPrivate struct fools us into believing it could
> be used elsewhere throughout the life of the domain. I now have second doubts
> about this. What do you think?
I agree with you, and since there a libxl_device_nic_list as you suggested, it
would actually be much cleaner and safer compared to libxl_domain_config
alternative (though with a small performance cost). And we would avoid end up
having config just lying there with no additional use (besides StartCallback)
and inconsistent info.
The only thing that the libxlDomainObjPrivate approach is better than
libxl_device_nic_list() would be that we don't need to refetch the devid, since
the nics array has it correctly filled already when console callback is invoked.
Whereas libxl_device_nic_list will refetch the same info (in additiona to all
entries in the backend directory) from xenstore thus adding up overhead. But
given that this is only once and in domain create I think it's not a big deal.
Right. I think the extra overhead is in the noise relative to the other
activities involved in starting a domain.
Would you agree then to resend this series without this patch and
using
libxl_device_nic_list, as the final approach? Thanks for pointing out this issue!
I think so. If you dislike the extra overhead of libxl_device_nic_list, another
option would be something like a libxlDomainStartCallbackInfo struct that
contains the virDomainObj and libxl_domain_config, and is passed to the start
callback via for_callback of libxl_asyncop_how. That would allow us to use the
libxl_domain_config object in the callback, but still dispose it after the start
completes.
Regards,
Jim
11:04 a.m.
New subject: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/23/2015 03:35 PM, Jim Fehlig wrote:
On 11/20/2015 05:40 PM, Joao Martins wrote:
>
> On 11/20/2015 07:05 PM, Jim Fehlig wrote:
>> On 11/19/2015 04:45 PM, Joao Martins wrote:
>>
>> You're not going to be happy with me...
>>
>>> This new field in libxlDomainObjPrivate is named "config"
>>> and is kept while the domain is active.
>> While this sounded like a good idea when I mentioned it, I'm now worried
that
>> the config will quickly become stale and cause problems if used elsewhere (e.g.
>> see my yet-to-be-written comment in 3/3). IIUC correctly, libxl_domain_config
>> is only useful when creating the domain. Subsequently adding/removing devices,
>> memory, vcpus, etc. would not be reflected in the libxl_domain_config object. I
>> suppose it would useful (and still valid) in the start callback, but IMO
>> including it in the libxlDomainPrivate struct fools us into believing it could
>> be used elsewhere throughout the life of the domain. I now have second doubts
>> about this. What do you think?
> I agree with you, and since there a libxl_device_nic_list as you suggested, it
> would actually be much cleaner and safer compared to libxl_domain_config
> alternative (though with a small performance cost). And we would avoid end up
> having config just lying there with no additional use (besides StartCallback)
> and inconsistent info.
>
> The only thing that the libxlDomainObjPrivate approach is better than
> libxl_device_nic_list() would be that we don't need to refetch the devid, since
> the nics array has it correctly filled already when console callback is invoked.
> Whereas libxl_device_nic_list will refetch the same info (in additiona to all
> entries in the backend directory) from xenstore thus adding up overhead. But
> given that this is only once and in domain create I think it's not a big deal.
Right. I think the extra overhead is in the noise relative to the other
activities involved in starting a domain.
> Would you agree then to resend this series without this patch and using
> libxl_device_nic_list, as the final approach? Thanks for pointing out this issue!
I think so. If you dislike the extra overhead of libxl_device_nic_list, another
option would be something like a libxlDomainStartCallbackInfo struct that
contains the virDomainObj and libxl_domain_config, and is passed to the start
callback via for_callback of libxl_asyncop_how. That would allow us to use the
libxl_domain_config object in the callback, but still dispose it after the start
completes.
I did a quick measurement to double-check and have a rough idea of the
"libxl_device_nic_list" cost.
Each line is in the form of
<n> VIFs: <libxl_device_nic_list cost in us> / <libvirt dom create cost in
secs>
1 VIF : 1066 us / 0.315 s
2 VIF : 1762 us / 0.375 s
4 VIF : 3528 us / 0.560 s
8 VIF : 6726 us / 0.953 s
16 VIF : 13378 us / 1.653 s
It almost grows linearly with the number of NICs having ~1ms per NIC. And given
the numbers above, I think the extra overhead is indeed small and neglible, so
I'll be sending with the libxl_device_nic_list approach as also agreed in your
previous comment.
Regards,
Joao
Regards,
Jim
5:45 p.m.
New subject: [libvirt] [PATCH 2/3] libxl: rename libxlConsoleCallback
. to a more generic name i.e. libxlDomainStartCallback,
since it will now cover another case other than the console.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
src/libxl/libxl_domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 60ef3a0..f0746f8 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -859,7 +859,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config)
}
static void
-libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
+libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
size_t i;
@@ -997,7 +997,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
priv->config = d_config;
aop_console_how.for_callback = vm;
- aop_console_how.callback = libxlConsoleCallback;
+ aop_console_how.callback = libxlDomainStartCallback;
if (restore_fd < 0) {
ret = libxl_domain_create_new(cfg->ctx, d_config,
&domid, NULL, &aop_console_how);
--
2.1.4
5:45 p.m.
New subject: [libvirt] [PATCH 3/3] libxl: implement virDomainInterfaceStats
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(a)oracle.com>
---
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;
+ }
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 */
--
2.1.4
1:25 p.m.
New subject: [libvirt] [PATCH 3/3] libxl: implement virDomainInterfaceStats
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(a)oracle.com>
---
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 */
6:40 p.m.
New subject: [libvirt] [PATCH 3/3] libxl: implement virDomainInterfaceStats
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(a)oracle.com>
> ---
> 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 */
3290
days inactive
3294
days old
9 comments
2 participants
participants (2)
-
Jim Fehlig -
Joao Martins