Page MenuHomeFreeBSD

netgraph/ng_bridge: learn MACs via control message
Needs ReviewPublic

Authored by donner on Feb 6 2021, 9:58 PM.

Details

Reviewers
kp
bcr
Group Reviewers
network
manpages
Summary

Add a new control message to move ethernet addresses to a given link
in ng_bridge(4). Send this message instead of doing the work directly.
This decouples the read-only activity from the modification under a
more strict writer lock.

Decoupling the work is a prerequisite for multithreaded operation.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 37109
Build 33998: arc lint + arc unit

Event Timeline

donner requested review of this revision.Feb 6 2021, 9:58 PM
  • Rebase after some changes
kp added a subscriber: kp.

I'm (mostly) happy with his from a style(9) perspective, but I don't understand the ng_bridge code well enough to have a well-informed opinion on the functionality.

sys/netgraph/ng_bridge.h
143

ETHER_ADDR_LEN

This revision is now accepted and ready to land.Feb 11 2021, 5:28 PM
  • Sanity checks on input

Don't insert an address, which is already present.

This revision now requires review to proceed.Feb 11 2021, 10:07 PM
  • New code uses current best practise for constants.
donner added inline comments.
sys/netgraph/ng_bridge.c
973–976

Avoid code duplication.

  • Add documentation in man page
afedorov added inline comments.
sys/netgraph/ng_bridge.h
163

I think the word "move" suggests a source and a destination. Maybe NGM_BRIDGE_ADD_HOST is more appropriate?

donner added inline comments.
sys/netgraph/ng_bridge.h
163

The review is split into two parts, which together build the "move".
This way the review is easier for people, which are not directly involved.

See "Stack" tab.

Did you mean D28559?

I figured out that if a loop is detected, this netgraph message moves the host from one link to another.

But, you are adding a public API/ABI that is visible from user space. This way people can use it to add a host to the specific link:

ngctl msg bridge: movehost <MAC> <hook>

Here, I think, it is better to use generally accepted conventions.

"Move" is like:

# mv <src> <dst>

or

memmove (void * dst, const void * src, size_t len);

The proposed message has add/put semantics.

P/S. If you don't want to add a public API, you may use the ng_send_fn () function, which also forces write semantics for the node.

donner marked an inline comment as done.EditedFeb 18 2021, 7:41 PM

Did you mean D28559?

Yes.

I figured out that if a loop is detected, this netgraph message moves the host from one link to another.

Yes, that's part of the "move". This kind of loop detection is taken as given.

But, you are adding a public API/ABI that is visible from user space. This way people can use it to add a host to the specific link:

ngctl msg bridge: movehost <MAC> <hook>

Nope. Usually the command is using a struct:

ngctl msg bridge: movehost { addr = 00:00:00:01:02:03 , hook = "link17" }

Even if you assume a "direction" in the order of arguments you usually say "move the address to the hook".

The proposed message has add/put semantics.

Only for the first part.

P/S. If you don't want to add a public API, you may use the ng_send_fn () function, which also forces write semantics for the node.

I considered this option of private messaging, but I found some situations, that an external injection is helpful of practical operations.

Example: If you have to destroy and rebuild a netgraph network running in production, you can backup the gettable output and reinject this after recreating the node/network. I do have this situation.

I'm not an English expert, so you may be right.
Technically, I have no questions, I think this patch can be committed.