12:56 p.m.
Hey,
This patch series implements virDomainInterfaceStats
but based on "ConsoleCallback" as opposed of doing it in libxlDomainStart.
The series is divided as following: Patch 1 renames console callback to
something more generic and Patch 2 implements virDomainInterfaceStats also
taking the previous review (v3 statistics) comments.
Changes since v1:
- Improve error reporting in case interface is not known
- Use libxl_device_nic_list as opposed to libxl_domain_config
Regards,
Joao
Joao Martins (2):
libxl: rename libxlConsoleCallback
libxl: implement virDomainInterfaceStats
src/libxl/libxl_domain.c | 33 ++++++++++++++++++++++++++++--
src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 2 deletions(-)
--
2.1.4
12:56 p.m.
New subject: [libvirt] [PATCH v2 1/2] 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 40dcea1..a7267b0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -854,7 +854,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;
@@ -988,7 +988,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virObjectUnlock(vm);
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
6:08 p.m.
New subject: [libvirt] [PATCH v2 1/2] libxl: rename libxlConsoleCallback
On 11/23/2015 11:56 AM, Joao Martins wrote:
. 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 40dcea1..a7267b0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -854,7 +854,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;
@@ -988,7 +988,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virObjectUnlock(vm);
aop_console_how.for_callback = vm;
- aop_console_how.callback = libxlConsoleCallback;
+ aop_console_how.callback = libxlDomainStartCallback;
Before pushing, I wanted to change the 'aop_console_how' variable to something
more generic too, but realized it is the 'const libxl_asyncprogress_how
*aop_console_how' parameter to libxl_domain_create_{new,restore}. AFAICT, this
callback is invoked when a console becomes available for the domain. It might be
possible that network devices have not been created (devid = -1) when the
callback is invoked. I thought about adding a separate libxlDomainStartCallback
and registering it with the 'const libxl_asyncop_how *ao_how' parameter, but
this would change the synchronous behavior of libxlDomainStart and be quite a
bit more intrusive.
In the end, I think it is best to explicitly call a function that creates the
ifnames after a successful libxl_domain_create_{new,restore}. See my reply to
2/2 for an example of this idea.
Regards,
Jim
7:02 a.m.
New subject: [libvirt] [PATCH v2 1/2] libxl: rename libxlConsoleCallback
On 12/02/2015 12:08 AM, Jim Fehlig wrote:
On 11/23/2015 11:56 AM, Joao Martins wrote:
> . 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 40dcea1..a7267b0 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -854,7 +854,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;
> @@ -988,7 +988,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
> virObjectUnlock(vm);
>
> aop_console_how.for_callback = vm;
> - aop_console_how.callback = libxlConsoleCallback;
> + aop_console_how.callback = libxlDomainStartCallback;
Before pushing, I wanted to change the 'aop_console_how' variable to something
more generic too, but realized it is the 'const libxl_asyncprogress_how
*aop_console_how' parameter to libxl_domain_create_{new,restore}. AFAICT, this
callback is invoked when a console becomes available for the domain. It might be
possible that network devices have not been created (devid = -1) when the
callback is invoked. I thought about adding a separate libxlDomainStartCallback
and registering it with the 'const libxl_asyncop_how *ao_how' parameter, but
this would change the synchronous behavior of libxlDomainStart and be quite a
bit more intrusive.
I see, I wasn't aware of that.
In the end, I think it is best to explicitly call a function that creates the
ifnames after a successful libxl_domain_create_{new,restore}. See my reply to
2/2 for an example of this idea.
OK, Thanks for all the feedback!
Regards,
Joao
Regards,
Jim
12:57 p.m.
New subject: [libvirt] [PATCH v2 2/2] 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 successful 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].
We extract the network interfaces info from libxl in
libxlDomainStartCallback() and make ifname . 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:
- Use libxl_device_nic_list() for getting each network interface
devid in DomainStartCallback.
- Improve error reporting by appropriately setting the right error
when no interface is known.
- 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 | 29 +++++++++++++++++++++++++++
src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 81 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a7267b0..141f241 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,6 +728,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);
@@ -857,6 +868,8 @@ static void
libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
+ libxl_device_nic *nics;
+ int nnics;
size_t i;
virObjectLock(vm);
@@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
VIR_FREE(console);
}
}
+
+ if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
+ for (i = 0; i < vm->def->nnets && i < nnics; i++) {
+ virDomainNetDefPtr net = vm->def->nets[i];
+ libxl_device_nic *x_nic = &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..55d991b 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,56 @@ 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 = 0;
+ break;
+ }
+ }
+
+ if (ret == 0)
+ ret = virNetInterfaceStats(path, stats);
+ else
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("'%s' is not a known interface"), path);
+
+ 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 +5472,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
6:45 p.m.
New subject: [libvirt] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats
On 11/23/2015 11:57 AM, 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 successful 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].
One interesting Xen behavior that has existing for many, many years is that a PV
nic is implicitly created for each emulated nic specified in the config. The
guest OS picks which one to use. These days most will use the PV nic, and if
they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
guest with
<interface type='bridge'>
<source bridge='br0'/>
<mac address='00:16:3e:7a:35:ce'/>
<script path='/etc/xen/scripts/vif-bridge'/>
</interface>
results in two vif devices on the host
# ip a | grep vif
607: vif519.0-emu: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 500
608: vif519.0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 512
both are connected to the bridge
# brctl show br0
bridge name bridge id STP enabled interfaces
br0 8000.001e676598f5 no eth0
vif519.0
vif519.0-emu
In this case, the (not nice) guest OS is using the PV nic but did not unplug the
emulated one. So we have two interfaces, but the virDomainDef only contains one
# virsh domiflist 519
Interface Type Source Model MAC
-------------------------------------------------------
vif519.0-emu bridge br0 - 00:16:3e:7a:35:ce
Not a fault of this patch, but we'll need to figure out how to handle the
implicitly created PV nic. The interesting case is identifying emulated nics
that have been unplugged by a nice guest, and hence no longer exist in the host
(e.g. vif519.0-emu in the above example).
We extract the network interfaces info from libxl in
libxlDomainStartCallback() and make ifname . 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:
- Use libxl_device_nic_list() for getting each network interface
devid in DomainStartCallback.
- Improve error reporting by appropriately setting the right error
when no interface is known.
- 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 | 29 +++++++++++++++++++++++++++
src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 81 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a7267b0..141f241 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
}
+ if ((vm->def->nnets)) {
+ ssize_t i;
size_t
+
+ 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);
@@ -857,6 +868,8 @@ static void
libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
+ libxl_device_nic *nics;
+ int nnics;
size_t i;
virObjectLock(vm);
@@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
VIR_FREE(console);
}
}
+
+ if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
+ for (i = 0; i < vm->def->nnets && i < nnics; i++) {
+ virDomainNetDefPtr net = vm->def->nets[i];
+ libxl_device_nic *x_nic = &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;
+ }
+ }
As mentioned in my reply to 1/2, I think we should simply create the names after
a successful libxl_domain_create_{new,restore}. Aside from the implicit PV nic
issue, the patch works for me with the below diff squashed in. What do you think
of this change? Does it work for you?
Regards,
Jim
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index b45cd27..086406b 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -729,7 +729,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
if ((vm->def->nnets)) {
- ssize_t i;
+ size_t i;
for (i = 0; i < vm->def->nnets; i++) {
virDomainNetDefPtr net = vm->def->nets[i];
@@ -868,8 +868,6 @@ static void
libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
- libxl_device_nic *nics;
- int nnics;
size_t i;
virObjectLock(vm);
@@ -897,25 +895,35 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
}
}
- if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
- for (i = 0; i < vm->def->nnets && i < nnics; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
- libxl_device_nic *x_nic = &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);
}
+/*
+ * Create interface names for the network devices in parameter def.
+ * Names are created with the pattern
'vif<domid>.<devid><suffix>'.
+ * devid is extracted from the network devices in the d_config
+ * parameter. User-provided interface names are skipped.
+ */
+static void
+libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ size_t i;
+
+ for (i = 0; i < def->nnets && i < d_config->num_nics; i++) {
+ virDomainNetDefPtr net = 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;
+
+ ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s",
+ def->id, x_nic->devid, suffix));
+ }
+}
+
/*
* Start a domain through libxenlight.
@@ -1056,6 +1064,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
goto cleanup_dom;
+ libxlDomainCreateIfaceNames(vm->def, &d_config);
if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
goto cleanup_dom;
j
7:02 a.m.
New subject: [libvirt] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats
On 12/02/2015 12:45 AM, Jim Fehlig wrote:
On 11/23/2015 11:57 AM, 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 successful 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].
One interesting Xen behavior that has existing for many, many years is that a PV
nic is implicitly created for each emulated nic specified in the config. The
guest OS picks which one to use. These days most will use the PV nic, and if
they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
guest with
<interface type='bridge'>
<source bridge='br0'/>
<mac address='00:16:3e:7a:35:ce'/>
<script path='/etc/xen/scripts/vif-bridge'/>
</interface>
results in two vif devices on the host
# ip a | grep vif
607: vif519.0-emu: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 500
608: vif519.0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 512
both are connected to the bridge
# brctl show br0
bridge name bridge id STP enabled interfaces
br0 8000.001e676598f5 no eth0
vif519.0
vif519.0-emu
In this case, the (not nice) guest OS is using the PV nic but did not unplug the
emulated one. So we have two interfaces, but the virDomainDef only contains one
# virsh domiflist 519
Interface Type Source Model MAC
-------------------------------------------------------
vif519.0-emu bridge br0 - 00:16:3e:7a:35:ce
Not a fault of this patch, but we'll need to figure out how to handle the
implicitly created PV nic. The interesting case is identifying emulated nics
that have been unplugged by a nice guest, and hence no longer exist in the host
(e.g. vif519.0-emu in the above example).
Indeed this is an issue I am aware of but wasn't sure on how to handle it.
The
way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
"model=netfront" since there wouldn't be an emulated nic. So perhaps we
could
differ it if this was specified on HVM guests. If that wasn't the case we would
add the complementary interface along with checking if indeed the net device
exists. On cleanup we would just delete the second interface that would appear
with the same name.
> We extract the network interfaces info from libxl in
> libxlDomainStartCallback() and make ifname . 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:
> - Use libxl_device_nic_list() for getting each network interface
> devid in DomainStartCallback.
> - Improve error reporting by appropriately setting the right error
> when no interface is known.
> - 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 | 29 +++++++++++++++++++++++++++
> src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index a7267b0..141f241 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
> }
> }
>
> + if ((vm->def->nnets)) {
> + ssize_t i;
size_t
Apologies for the typo.
> +
> + 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);
> @@ -857,6 +868,8 @@ static void
> libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
> {
> virDomainObjPtr vm = for_callback;
> + libxl_device_nic *nics;
> + int nnics;
> size_t i;
>
> virObjectLock(vm);
> @@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
> VIR_FREE(console);
> }
> }
> +
> + if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
> + for (i = 0; i < vm->def->nnets && i < nnics; i++) {
> + virDomainNetDefPtr net = vm->def->nets[i];
> + libxl_device_nic *x_nic = &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;
> + }
> + }
As mentioned in my reply to 1/2, I think we should simply create the names after
a successful libxl_domain_create_{new,restore}. Aside from the implicit PV nic
issue, the patch works for me with the below diff squashed in. What do you think
of this change? Does it work for you?
Looks great, Perhaps just changing the version to 1.3.0 since it's no longer
1.2.22.
Regards,
Joao
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b3987b9..aa81570 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5472,7 +5472,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
#endif
.nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
.nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
- .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
+ .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
.domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
.domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
.connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
Regards,
Jim
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index b45cd27..086406b 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -729,7 +729,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
if ((vm->def->nnets)) {
- ssize_t i;
+ size_t i;
for (i = 0; i < vm->def->nnets; i++) {
virDomainNetDefPtr net = vm->def->nets[i];
@@ -868,8 +868,6 @@ static void
libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
- libxl_device_nic *nics;
- int nnics;
size_t i;
virObjectLock(vm);
@@ -897,25 +895,35 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
}
}
- if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
- for (i = 0; i < vm->def->nnets && i < nnics; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
- libxl_device_nic *x_nic = &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);
}
+/*
+ * Create interface names for the network devices in parameter def.
+ * Names are created with the pattern
'vif<domid>.<devid><suffix>'.
+ * devid is extracted from the network devices in the d_config
+ * parameter. User-provided interface names are skipped.
+ */
+static void
+libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+ size_t i;
+
+ for (i = 0; i < def->nnets && i < d_config->num_nics; i++) {
+ virDomainNetDefPtr net = 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;
+
+ ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s",
+ def->id, x_nic->devid, suffix));
+ }
+}
+
/*
* Start a domain through libxenlight.
@@ -1056,6 +1064,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
goto cleanup_dom;
+ libxlDomainCreateIfaceNames(vm->def, &d_config);
if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
goto cleanup_dom;
j
12:50 p.m.
New subject: [libvirt] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats
On 12/02/2015 06:02 AM, Joao Martins wrote:
On 12/02/2015 12:45 AM, Jim Fehlig wrote:
> On 11/23/2015 11:57 AM, 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 successful 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].
> One interesting Xen behavior that has existing for many, many years is that a PV
> nic is implicitly created for each emulated nic specified in the config. The
> guest OS picks which one to use. These days most will use the PV nic, and if
> they are nice, "unplug" the emulated one via the unplug protocol. E.g. an
HVM
> guest with
>
> <interface type='bridge'>
> <source bridge='br0'/>
> <mac address='00:16:3e:7a:35:ce'/>
> <script path='/etc/xen/scripts/vif-bridge'/>
> </interface>
>
> results in two vif devices on the host
>
> # ip a | grep vif
> 607: vif519.0-emu: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> master br0 state UNKNOWN group default qlen 500
> 608: vif519.0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> master br0 state UNKNOWN group default qlen 512
>
> both are connected to the bridge
>
> # brctl show br0
> bridge name bridge id STP enabled interfaces
> br0 8000.001e676598f5 no eth0
> vif519.0
> vif519.0-emu
>
> In this case, the (not nice) guest OS is using the PV nic but did not unplug the
> emulated one. So we have two interfaces, but the virDomainDef only contains one
>
> # virsh domiflist 519
> Interface Type Source Model MAC
> -------------------------------------------------------
> vif519.0-emu bridge br0 - 00:16:3e:7a:35:ce
>
> Not a fault of this patch, but we'll need to figure out how to handle the
> implicitly created PV nic. The interesting case is identifying emulated nics
> that have been unplugged by a nice guest, and hence no longer exist in the host
> (e.g. vif519.0-emu in the above example).
>
Indeed this is an issue I am aware of but wasn't sure on how to handle it. The
way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
"model=netfront" since there wouldn't be an emulated nic.
Yep, libxl allows us specify a PV nic only, but not an emulated nic only.
So perhaps we could
differ it if this was specified on HVM guests. If that wasn't the case we would
add the complementary interface along with checking if indeed the net device
exists. On cleanup we would just delete the second interface that would appear
with the same name.
Right. So the running config would have the implicitly added PV device. The
persistent config wouldn't change. Are you volunteering to write a patch? :-).
>> We extract the network interfaces info from libxl in
>> libxlDomainStartCallback() and make ifname . 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:
>> - Use libxl_device_nic_list() for getting each network interface
>> devid in DomainStartCallback.
>> - Improve error reporting by appropriately setting the right error
>> when no interface is known.
>> - 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 | 29 +++++++++++++++++++++++++++
>> src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 81 insertions(+)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index a7267b0..141f241 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>> }
>> }
>>
>> + if ((vm->def->nnets)) {
>> + ssize_t i;
> size_t
>
Apologies for the typo.
>> +
>> + 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);
>> @@ -857,6 +868,8 @@ static void
>> libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>> {
>> virDomainObjPtr vm = for_callback;
>> + libxl_device_nic *nics;
>> + int nnics;
>> size_t i;
>>
>> virObjectLock(vm);
>> @@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev,
void *for_callback)
>> VIR_FREE(console);
>> }
>> }
>> +
>> + if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL)
{
>> + for (i = 0; i < vm->def->nnets && i < nnics; i++) {
>> + virDomainNetDefPtr net = vm->def->nets[i];
>> + libxl_device_nic *x_nic = &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;
>> + }
>> + }
> As mentioned in my reply to 1/2, I think we should simply create the names after
> a successful libxl_domain_create_{new,restore}. Aside from the implicit PV nic
> issue, the patch works for me with the below diff squashed in. What do you think
> of this change? Does it work for you?
>
Looks great, Perhaps just changing the version to 1.3.0 since it's no longer 1.2.22.
I changed that when resolving a conflict that arose from dropping patch 1. I'm
going to push this patch with my diff squashed in. It has been on the list for
quite some time, has actually went through 4 versions as your patch history
notes indicate, and IMO has been well vetted and tested.I think it is an
appropriate patch to push even though we entered the 1.3.0 freeze today.
Thanks a lot for your patience!
Regards,
Jim
Regards,
Joao
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b3987b9..aa81570 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5472,7 +5472,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
#endif
.nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
.nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
- .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
+ .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
.domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
.domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
.connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
> Regards,
> Jim
>
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index b45cd27..086406b 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -729,7 +729,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
> }
>
> if ((vm->def->nnets)) {
> - ssize_t i;
> + size_t i;
>
> for (i = 0; i < vm->def->nnets; i++) {
> virDomainNetDefPtr net = vm->def->nets[i];
> @@ -868,8 +868,6 @@ static void
> libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
> {
> virDomainObjPtr vm = for_callback;
> - libxl_device_nic *nics;
> - int nnics;
> size_t i;
>
> virObjectLock(vm);
> @@ -897,25 +895,35 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void
> *for_callback)
> }
> }
>
> - if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
> - for (i = 0; i < vm->def->nnets && i < nnics; i++) {
> - virDomainNetDefPtr net = vm->def->nets[i];
> - libxl_device_nic *x_nic = &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);
> }
>
> +/*
> + * Create interface names for the network devices in parameter def.
> + * Names are created with the pattern
'vif<domid>.<devid><suffix>'.
> + * devid is extracted from the network devices in the d_config
> + * parameter. User-provided interface names are skipped.
> + */
> +static void
> +libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nnets && i < d_config->num_nics; i++)
{
> + virDomainNetDefPtr net = 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;
> +
> + ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s",
> + def->id, x_nic->devid, suffix));
> + }
> +}
> +
>
> /*
> * Start a domain through libxenlight.
> @@ -1056,6 +1064,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
> virDomainObjPtr vm,
> if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
> goto cleanup_dom;
>
> + libxlDomainCreateIfaceNames(vm->def, &d_config);
>
> if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
> goto cleanup_dom;
> j
>
1:26 p.m.
New subject: [libvirt] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats
On Wed, Dec 02, 2015 at 11:50:41AM -0700, Jim Fehlig wrote:
On 12/02/2015 06:02 AM, Joao Martins wrote:
>
> On 12/02/2015 12:45 AM, Jim Fehlig wrote:
>> On 11/23/2015 11:57 AM, 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 successful 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].
>> One interesting Xen behavior that has existing for many, many years is that a
PV
>> nic is implicitly created for each emulated nic specified in the config. The
>> guest OS picks which one to use. These days most will use the PV nic, and if
>> they are nice, "unplug" the emulated one via the unplug protocol. E.g.
an HVM
>> guest with
>>
>> <interface type='bridge'>
>> <source bridge='br0'/>
>> <mac address='00:16:3e:7a:35:ce'/>
>> <script path='/etc/xen/scripts/vif-bridge'/>
>> </interface>
>>
>> results in two vif devices on the host
>>
>> # ip a | grep vif
>> 607: vif519.0-emu: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
pfifo_fast
>> master br0 state UNKNOWN group default qlen 500
>> 608: vif519.0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
pfifo_fast
>> master br0 state UNKNOWN group default qlen 512
>>
>> both are connected to the bridge
>>
>> # brctl show br0
>> bridge name bridge id STP enabled interfaces
>> br0 8000.001e676598f5 no eth0
>> vif519.0
>> vif519.0-emu
>>
>> In this case, the (not nice) guest OS is using the PV nic but did not unplug
the
>> emulated one. So we have two interfaces, but the virDomainDef only contains one
>>
>> # virsh domiflist 519
>> Interface Type Source Model MAC
>> -------------------------------------------------------
>> vif519.0-emu bridge br0 - 00:16:3e:7a:35:ce
>>
>> Not a fault of this patch, but we'll need to figure out how to handle the
>> implicitly created PV nic. The interesting case is identifying emulated nics
>> that have been unplugged by a nice guest, and hence no longer exist in the host
>> (e.g. vif519.0-emu in the above example).
>>
> Indeed this is an issue I am aware of but wasn't sure on how to handle it. The
> way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
> "model=netfront" since there wouldn't be an emulated nic.
Yep, libxl allows us specify a PV nic only, but not an emulated nic only.
> So perhaps we could
> differ it if this was specified on HVM guests. If that wasn't the case we would
> add the complementary interface along with checking if indeed the net device
> exists. On cleanup we would just delete the second interface that would appear
> with the same name.
Right. So the running config would have the implicitly added PV device. The
persistent config wouldn't change. Are you volunteering to write a patch? :-).
Yeap :-D
>
>>> We extract the network interfaces info from libxl in
>>> libxlDomainStartCallback() and make ifname . 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:
>>> - Use libxl_device_nic_list() for getting each network interface
>>> devid in DomainStartCallback.
>>> - Improve error reporting by appropriately setting the right error
>>> when no interface is known.
>>> - 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 | 29 +++++++++++++++++++++++++++
>>> src/libxl/libxl_driver.c | 52
++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 81 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>> index a7267b0..141f241 100644
>>> --- a/src/libxl/libxl_domain.c
>>> +++ b/src/libxl/libxl_domain.c
>>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>> }
>>> }
>>>
>>> + if ((vm->def->nnets)) {
>>> + ssize_t i;
>> size_t
>>
> Apologies for the typo.
>
>>> +
>>> + 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);
>>> @@ -857,6 +868,8 @@ static void
>>> libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void
*for_callback)
>>> {
>>> virDomainObjPtr vm = for_callback;
>>> + libxl_device_nic *nics;
>>> + int nnics;
>>> size_t i;
>>>
>>> virObjectLock(vm);
>>> @@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event
*ev, void *for_callback)
>>> VIR_FREE(console);
>>> }
>>> }
>>> +
>>> + if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) !=
NULL) {
>>> + for (i = 0; i < vm->def->nnets && i < nnics;
i++) {
>>> + virDomainNetDefPtr net = vm->def->nets[i];
>>> + libxl_device_nic *x_nic = &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;
>>> + }
>>> + }
>> As mentioned in my reply to 1/2, I think we should simply create the names
after
>> a successful libxl_domain_create_{new,restore}. Aside from the implicit PV nic
>> issue, the patch works for me with the below diff squashed in. What do you
think
>> of this change? Does it work for you?
>>
> Looks great, Perhaps just changing the version to 1.3.0 since it's no longer
1.2.22.
I changed that when resolving a conflict that arose from dropping patch 1. I'm
going to push this patch with my diff squashed in. It has been on the list for
quite some time, has actually went through 4 versions as your patch history
notes indicate, and IMO has been well vetted and tested.I think it is an
appropriate patch to push even though we entered the 1.3.0 freeze today.
Great! Many thanks for the time reviewing it!
Thanks a lot for your patience!
Regards,
Jim
>
> Regards,
> Joao
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b3987b9..aa81570 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5472,7 +5472,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
> #endif
> .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
> .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
> + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
> .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
> .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
> .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>
>
>> Regards,
>> Jim
>>
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index b45cd27..086406b 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -729,7 +729,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>> }
>>
>> if ((vm->def->nnets)) {
>> - ssize_t i;
>> + size_t i;
>>
>> for (i = 0; i < vm->def->nnets; i++) {
>> virDomainNetDefPtr net = vm->def->nets[i];
>> @@ -868,8 +868,6 @@ static void
>> libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>> {
>> virDomainObjPtr vm = for_callback;
>> - libxl_device_nic *nics;
>> - int nnics;
>> size_t i;
>>
>> virObjectLock(vm);
>> @@ -897,25 +895,35 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev,
void
>> *for_callback)
>> }
>> }
>>
>> - if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL)
{
>> - for (i = 0; i < vm->def->nnets && i < nnics; i++)
{
>> - virDomainNetDefPtr net = vm->def->nets[i];
>> - libxl_device_nic *x_nic = &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);
>> }
>>
>> +/*
>> + * Create interface names for the network devices in parameter def.
>> + * Names are created with the pattern
'vif<domid>.<devid><suffix>'.
>> + * devid is extracted from the network devices in the d_config
>> + * parameter. User-provided interface names are skipped.
>> + */
>> +static void
>> +libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config
*d_config)
>> +{
>> + size_t i;
>> +
>> + for (i = 0; i < def->nnets && i < d_config->num_nics;
i++) {
>> + virDomainNetDefPtr net = 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;
>> +
>> + ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s",
>> + def->id, x_nic->devid, suffix));
>> + }
>> +}
>> +
>>
>> /*
>> * Start a domain through libxenlight.
>> @@ -1056,6 +1064,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
>> virDomainObjPtr vm,
>> if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
>> goto cleanup_dom;
>>
>> + libxlDomainCreateIfaceNames(vm->def, &d_config);
>>
>> if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
>> goto cleanup_dom;
>> j
>>
3272
days inactive
3281
days old
8 comments
2 participants
participants (2)
-
Jim Fehlig -
Joao Martins