[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] lxc: stop incorrectly validating interface type



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 laine org>
> >> ---
> >>  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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]