Page MenuHomeFreeBSD

libifconfig: Add initial support for if_bridge
Needs ReviewPublic

Authored by marieheleneka_gmail.com on Oct 11 2018, 4:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:59 AM
Unknown Object (File)
Oct 30 2024, 9:41 AM
Unknown Object (File)
Oct 12 2024, 8:18 AM
Unknown Object (File)
Sep 27 2024, 2:52 PM
Unknown Object (File)
Sep 18 2024, 2:22 PM
Unknown Object (File)
Sep 8 2024, 7:24 PM
Unknown Object (File)
Sep 8 2024, 12:47 AM
Unknown Object (File)
Sep 6 2024, 3:12 AM
Subscribers

Details

Reviewers
allanjude
kp
Summary

Implements support for adding and removing members to/from bridges.
Fixed typo in some docxml comments
Submitted by: Marie Helene Kvello-Aune (freebsd@mhka.no)
MFC after: 30d

Not to be committed until after 12 has branched.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20176
Build 19660: arc lint + arc unit

Event Timeline

I’ll try to review in the next day or two, but it needs tests ;) (in a separte commit)
I think there are currently no functional tests for if_bridge, but it should be pretty straightforward to spin up a vnet jail, create a bridge in it and then use libifconfig to add/remove interfaces.
It’d have the very nice side-effect of getting us some basic bridge tests too.

lib/libifconfig/Makefile
19

(Style) You've got trailing spaces here.

lib/libifconfig/libifconfig.h
262

(Style) You've got trailing spaces here.

lib/libifconfig/libifconfig_bridge.c
55

(Style) This line is too long.

66

(Style) This line is too long.

(Idle musing) How much detail do we get from the bridge ioctl code? Could we, for example, distinguish 'interface to add doesn't exist' from 'bridge doesn't exist' from 'access denied'?

78

I wonder if it's worth checking that the user isn't doing silly things, like passing in NULL pointers, or overly long interface names (> IFNAMSIZ).

That wasn't really a concern in ifconfig, but when others start using this we may want more validation.

(Certainly not a blocking issue.)

marieheleneka_gmail.com marked 8 inline comments as done.
marieheleneka_gmail.com added inline comments.
lib/libifconfig/Makefile
19

Made the list one entry per line, to reduce size of future diffs.

lib/libifconfig/libifconfig_bridge.c
66

I'm not sure exactly how much detail we can get from this, but the error code is stored as-is by ifconfig_ioctlwrap, and can be retrieved using the ifconfig_err_* functions.

78

We might want to do that. This should probably also be done for various other parameters, like description length. I'll add it to the todo list.

marieheleneka_gmail.com marked 3 inline comments as done.

Update according to feedback

Some minor corrections. Turns out it helps if I run tests on the right tree. :)

lib/libifconfig/libifconfig_bridge.c
72

This can probably be a bool. It's only ever going to be true or false.

It might also be helpful to make this part of the public API of libifconfig. I can certainly imagine situations where I'd rather use this than if (add) ifconfig_bridge_add_member(); else ifconfig_bridge_remove_member().
The counterargument to that is that it might be better to have a smaller API. Your call :)

marieheleneka_gmail.com added inline comments.
lib/libifconfig/libifconfig_bridge.c
72

I contemplated not implementing the ifconfig_bridge_add_member() and ifconfig_bridge_remove_member(), and rather have the static ifconfig_bridge_addrem_member() be part of the API as ifconfig_bridge_modify_member(), with the current arguments. IIRC, there are a few other parts of the API which could benefit from such consolidation as well; I'll have to look closer at it.

I like the idea of using bool, but in that case, it should probably be used for every situation where it's really just a boolean.

I'll ponder some on the two above points and make a decision on how to do this. At least it's easy to implement the decision, as libifconfig is marked private and therefore doesn't need API/ABI compatibility between releases.

This revision is now accepted and ready to land.Nov 27 2018, 11:38 AM
This revision now requires review to proceed.Nov 27 2018, 11:41 AM