On Thu, Dec 06, 2018 at 09:37:09AM -0500, Laine Stump wrote: > 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@xxxxxxxxx> > >> --- > >> 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. My preference is always to be explicit with the validation in a switch() unless something earlier *in the same function scope* has already done validation. IOW don't rely on the caller having previously called something else to do validation, as that's fragile when code is refactored. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
