drivers/ipvlan: add ipvlan_flag option, support l3s ipvlan_mode#42542
drivers/ipvlan: add ipvlan_flag option, support l3s ipvlan_mode#42542cpuguy83 merged 1 commit intomoby:mastermoby/moby:masterfrom zhangyoufu:libnetwork-ipvlan-enhanceCopy head branch name to clipboard
Conversation
Signed-off-by: Youfu Zhang <zhangyoufu@gmail.com>
cpuguy83
left a comment
There was a problem hiding this comment.
One question.
Mostly seems fine though I'm not a network expert.
| ParentIndex: parentLink.Attrs().Index, | ||
| }, | ||
| Mode: mode, | ||
| Flag: flag, |
There was a problem hiding this comment.
It seems like we are always setting this now.
Is there a behavior change here?
There was a problem hiding this comment.
No behavior change here. Bridge (0) is the traditional mode where slaves can cross-talk among
themselves apart from talking through the master device.
|
@evol262 ptal |
| parentOpt = "parent" // parent interface -o parent | ||
| driverModeOpt = ipvlanType + "_mode" // mode -o ipvlan_mode | ||
| driverFlagOpt = ipvlanType + "_flag" // flag -o ipvlan_flag | ||
|
|
There was a problem hiding this comment.
It will get optimized out by the compiler anyway, but it seems like these could simly be driverModeOpt = "ipvlan_mode", since the variable never appears be reassigned anywhere. Honestly, it could probably be a const, but that's nitpicky. Still, the concatenation should just be a string literal, otherwise a reader may spend time looking through the codebase to see if it ever changes.
There was a problem hiding this comment.
There are many references of ipvlanType all around this driver, all of them are redundant. Considering the similarity between macvlan and ipvlan, maybe the original author of the code intentionally leave an extra level of indirection to make code refactoring (identifier renaming) easier and more confident. I tried to keep the original style when introducing my changes.
|
LGTM in general. A small nitpick, but not worth blocking on. |
|
The commit in torvalds/linux didn't match for some reason; I think this was the one referred to; torvalds/linux@4fbae7d |
Fixes moby#42542 Signed-off-by: Youfu Zhang <zhangyoufu@gmail.com> (cherry picked from commit 549d24b) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
recreated from moby/libnetwork#2619