Fix bug in ifconfig preventing proper vlan creation
ClosedPublic
Actions

Authored by • hselasky on Dec 9 2020, 11:45 AM.

Details

Reviewers

kib
ae
melifaro
bz
kevans

Commits

rS368505: Fix bug in ifconfig regarding VLAN creation.
rS368504: Fix bug in ifconfig regarding VLAN creation.
rS368502: Fix bug in ifconfig preventing proper VLAN creation.
rS368492: Fix bug in ifconfig preventing proper VLAN creation.

Summary

Detection of interface type by filter must happen before detection of interface type by prefix. Else the following sequence of commands will try to create a LAGG interface instead of a VLAN interface, which accidentially worked previously, because the date pointed to by the ifr_data pointer was not parsed for VLAN's. This is a regression after r366917 (adding MT_FILTER) and r368229 (VLAN creation now parses the ifr_data field).

ifconfig lagg0 create
ifconfig lagg0.256 create

MFC after: 3 days
Sponsored by: Mellanox Technologies // NVIDIA Networking

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 35273

Event Timeline

• hselasky created this revision.Dec 9 2020, 11:45 AM

Herald added a subscriber: imp. · View Herald TranscriptDec 9 2020, 11:45 AM

• hselasky requested review of this revision.Dec 9 2020, 11:45 AM

Harbormaster completed remote builds in B35272: Diff 80474.Dec 9 2020, 11:45 AM

• hselasky added reviewers: melifaro, bz.Dec 9 2020, 11:45 AM

• hselasky edited the summary of this revision. (Show Details)Dec 9 2020, 11:52 AM

kib accepted this revision.Dec 9 2020, 1:56 PM

kib added inline comments.

sbin/ifconfig/ifclone.c
137	Extra ()
144–145	Extra ()

This revision is now accepted and ready to land.Dec 9 2020, 1:56 PM

Address comments from Konstantin.

This revision now requires review to proceed.Dec 9 2020, 3:17 PM

Harbormaster completed remote builds in B35273: Diff 80478.Dec 9 2020, 3:17 PM

• hselasky marked 2 inline comments as done.Dec 9 2020, 3:17 PM

• hselasky added a reviewer: kevans.Dec 9 2020, 3:33 PM

I would have perhaps kept the old structure (still dropping the weird tautological comparison) so we only walk once and explore both possibilities in one loop (just stop looking at prefix-based callbacks if we found one, stop looking at filter-based callbacks if we found one, stop if we found one of each), but this is fine too and probably less error prone. There's also not much benefit to grabbing both for any in-tree stuff.

This revision is now accepted and ready to land.Dec 9 2020, 3:48 PM

stop if we found one of each

That's exactly what happens. lagg0.256 matches both lagg prefix and the vlan filter.

In D27521#615196, @hselasky wrote:

stop if we found one of each

That's exactly what happens. lagg0.256 matches both lagg prefix and the vlan filter.

Right, so in the consolidated view we'd have dcp_prefix and dcp_filter if it matched one of each, then make a decision based on that.

@kevans: I think the current code is pretty clear. I'm not sure how much we'll gain introducing another variable ...

Closed by commit rS368492: Fix bug in ifconfig preventing proper VLAN creation. (authored by • hselasky). · Explain WhyDec 9 2020, 8:13 PM

This revision was automatically updated to reflect the committed changes.

• hselasky added a commit: rS368492: Fix bug in ifconfig preventing proper VLAN creation..

• hselasky added a commit: rS368502: Fix bug in ifconfig preventing proper VLAN creation..Dec 10 2020, 9:30 AM

• hselasky added a commit: rS368504: Fix bug in ifconfig regarding VLAN creation..Dec 10 2020, 9:37 AM

• hselasky added a commit: rS368505: Fix bug in ifconfig regarding VLAN creation..Dec 10 2020, 9:39 AM