[libvirt] [PATCH v2 0/2] Fix 2 small LXC network interface bugs

[Laine Stump]

danpb suggested that I shouldn't completely remove the validation of
interface type I had removed in V1:

https://www.redhat.com/archives/libvir-list/2018-December/msg00120.html

This also re-orders the two patches, so the "check actual type" patch loses
its previous ACK due to modifications.


Laine Stump (2):
lxc: check actual type of interface not config type
lxc: don't forbid <interface type='direct'>

src/lxc/lxc_controller.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)

[Laine Stump]

Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() to return an error status
for unsupported interface types, but it erroneously put type='direct'
on the list of unsupported types.

type='direct' (implemented with a macvlan interface) is supported on
LXC, but it's interface shouldn't be placed on the list of interfaces
given to CreateMachineWithNetwork() because the interface is put
inside the container, while CreateMachineWithNetwork() only wants to
know about the parent veths of veth pairs (the parent veth remains on
the host side, while the child veth is put into the container).

Resolves: https://bugzilla.redhat.com/1656463
Signed-off-by: Laine Stump <***@laine.org>
---
src/lxc/lxc_controller.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 07342cbc77..cff004a034 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;

+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -388,6 +398,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
ctrl->nicindexes[ctrl->nnicindexes-1] = nicindex;
break;

+ case VIR_DOMAIN_NET_TYPE_DIRECT:
+ break;
+
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -395,7 +408,6 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
case VIR_DOMAIN_NET_TYPE_MCAST:
case VIR_DOMAIN_NET_TYPE_UDP:
case VIR_DOMAIN_NET_TYPE_INTERNAL:
- case VIR_DOMAIN_NET_TYPE_DIRECT:
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),

[Ján Tomko]

Post by Laine Stump
Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() to return an error status
for unsupported interface types, but it erroneously put type='direct'
on the list of unsupported types.
type='direct' (implemented with a macvlan interface) is supported on
LXC, but it's interface shouldn't be placed on the list of interfaces
given to CreateMachineWithNetwork() because the interface is put
inside the container, while CreateMachineWithNetwork() only wants to
know about the parent veths of veth pairs (the parent veth remains on
the host side, while the child veth is put into the container).
Resolves: https://bugzilla.redhat.com/1656463
---
src/lxc/lxc_controller.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 07342cbc77..cff004a034 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
+ /* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -388,6 +398,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
ctrl->nicindexes[ctrl->nnicindexes-1] = nicindex;
break;
+ break;
+
@@ -395,7 +408,6 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),

Technically, from the point of view of this function whether the net
type is supported or not does not matter (qemuBuildInterfaceCommandLine
uses 'break;' for all the other networks.

But any effort spent on this function would be better spent on removing
the QEMU/LXC duplication and not bothering calling it on non-systemd
systems, so:

Reviewed-by: Ján Tomko <***@redhat.com>

Jano

[Laine Stump]

virLXCControllerGetNICIndexes() was deciding whether or not to add the
ifindex for an interface's ifname to the list of ifindexes sent to
CreateMachineWithNetwork based on the interface type stored in the
config. This would be incorrect in the case of <interface
type='network'> where the network was giving out macvlan interfaces
tied to a physical device (i.e. when the actual interface type was
"direct").

Instead of checking the setting of "net->type", we should be checking
the setting of virDomainNetGetActualType(net).

I don't think this caused any actual misbehavior, it was just
technically wrong.

Signed-off-by: Laine Stump <***@laine.org>
---
src/lxc/lxc_controller.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..07342cbc77 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -367,7 +367,10 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
- switch (ctrl->def->nets[i]->type) {
+ virDomainNetType actualType
+ = virDomainNetGetActualType(ctrl->def->nets[i]);
+
+ switch (actualType) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -396,11 +399,11 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
+ virDomainNetTypeToString(actualType));
goto cleanup;
case VIR_DOMAIN_NET_TYPE_LAST:
default:
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
+ virReportEnumRangeError(virDomainNetType, actualType);
goto cleanup;
}
}

[Ján Tomko]

Post by Laine Stump
virLXCControllerGetNICIndexes() was deciding whether or not to add the
ifindex for an interface's ifname to the list of ifindexes sent to
CreateMachineWithNetwork based on the interface type stored in the
config. This would be incorrect in the case of <interface
type='network'> where the network was giving out macvlan interfaces
tied to a physical device (i.e. when the actual interface type was
"direct").
Instead of checking the setting of "net->type", we should be checking
the setting of virDomainNetGetActualType(net).
I don't think this caused any actual misbehavior, it was just
technically wrong.
---
src/lxc/lxc_controller.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..07342cbc77 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -367,7 +367,10 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
- switch (ctrl->def->nets[i]->type) {
+ virDomainNetType actualType
+ = virDomainNetGetActualType(ctrl->def->nets[i]);

Please join these two lines into one.

Post by Laine Stump
+
+ switch (actualType) {
@@ -396,11 +399,11 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
+ virDomainNetTypeToString(actualType));
goto cleanup;
- virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type);
+ virReportEnumRangeError(virDomainNetType, actualType);
goto cleanup;
}

Reviewed-by: Ján Tomko <***@redhat.com>

Jano