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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 20174 Build 19658: arc lint + arc unit
Event Timeline
Comment Actions
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.) | |
| 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. | |
| lib/libifconfig/libifconfig_bridge.c | ||
|---|---|---|
| 73 | 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(). | |
| lib/libifconfig/libifconfig_bridge.c | ||
|---|---|---|
| 73 | 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. | |