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() such that it returned error status for any interfaces that weren't implemented with a veth pair when it should have just been ignoring those interfaces. Since the interface type will have already been validated before reaching this function, we shouldn't be doing any validation at all - just add the ifindex of the parent veth if its a veth pair, and ignore it otherwise. Resolves: https://bugzilla.redhat.com/1656463 Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/lxc/lxc_controller.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index e853d02d65..1c20f451af 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; @@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl) 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"), - virDomainNetTypeToString(ctrl->def->nets[i]->type)); - goto cleanup; case VIR_DOMAIN_NET_TYPE_LAST: default: - virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type); - goto cleanup; + break; }

On 12/6/18 4:50 AM, Daniel P. Berrangé wrote: > On Wed, Dec 05, 2018 at 09:35:12PM -0500, Laine Stump wrote: >> 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() such that it returned >> error status for any interfaces that weren't implemented with a veth >> pair when it should have just been ignoring those interfaces. >> >> Since the interface type will have already been validated before >> reaching this function, we shouldn't be doing any validation at all - >> just add the ifindex of the parent veth if its a veth pair, and ignore >> it otherwise. >> >> Resolves: https://bugzilla.redhat.com/1656463 >> Signed-off-by: Laine Stump <laine(a)laine.org&gt; >> --- >> src/lxc/lxc_controller.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c >> index e853d02d65..1c20f451af 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; >> @@ -394,14 +404,9 @@ static int virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl) >> 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"), >> - virDomainNetTypeToString(ctrl->def->nets[i]->type)); >> - goto cleanup; >> case VIR_DOMAIN_NET_TYPE_LAST: >> default: >> - virReportEnumRangeError(virDomainNetType, ctrl->def->nets[i]->type); >> - goto cleanup; >> + break; >> } > This is removing too much. Only ethernet, bridge, network & direct are going > to work with LXC. All others should always result in an error message here, > and absolutely the LAST/default case must always report enum error. I had originally done what you suggest, then went back and changed it in order to not conflict with my commit comment saying that validation shouldn't even be done in this function, since the value had already been validated and doing it again would just be adding bulk to the code for no reason.