Add bind_intf argument to specify network interface for LAN commands #371
Conversation
|
Could you please explain why would anyone need that? Are the Linux routing rules not enough to establish the outbound interface? Why? |
|
Hi Alexander, This is helpful when using a non-default VRF, because we can pass the VRF interface name to this new option, and then the Linux routing rules for that specific VRF will be used. This is similar to the support provided by Some supporting docs are at https://docs.kernel.org/networking/vrf.html, which also notes "Processes can be “VRF aware” by binding a socket to the VRF device." Happy to take feedback on this change! I saw there are a few other places in the README / man page where the options are described, so I can work on getting those updated as part of this PR, if this is a feature you'd be open to accepting. Thanks for taking a look. |
|
Oh, I see now. Thank you. |
|
It would be nice to add that explanation (maybe briefer version) to the commit log as well. |
|
Also, it appears like your change is not compatible with macos and windows and must be conditionally disabled there. |
dvanallen
force-pushed
the
add_bind_intf_option
branch
from
1790baf
to
a3f118a
Compare
|
Force-pushed to get the commit description updated. Added conditional disablement and docs for the new option. Could you please kick off the test workflows again? |
src/plugins/ipmi_intf.c
Outdated
| @@ -399,6 +409,21 @@ ipmi_intf_socket_connect(struct ipmi_intf * intf) | |||
| continue; | |||
| } | |||
|
|
|||
| if (params->bind_intf != NULL) { | |||
| #ifdef SO_BINDTODEVICE | |||
There was a problem hiding this comment.
first, please place the #ifdef at the leftmost column.
second, if SO_BINDTODEVICE will ever change from a macro to an enum or const, this check will break. It's better to go the conventional feature-checking way, alter configure.ac with some extra AC_TRY_COMPILE to check if the code using SO_BINDTODEVICE can be compiled on the platform, then AC_DEFINE() some HAS_BINDTODEVICE macro, and then rely on that macro here.
Additional benefit to that approach is that you can check that autoconf definition in ipmitool.1.in as well and don't list in the man the option that is not supported on the platform.
There was a problem hiding this comment.
Thanks for the guidance here, and apologies for the delay in getting back.
I added the suggested AC_TRY_COMPILE macro, and used that throughout the source files, but I haven't found a good way to do a conditional change of the man page. I considered adding an output variable based on the same AC_TRY_COMPILE check, which would be empty or hold the option help. That doesn't seem to work for multi-line strings, and would move the option help string all the way into configure.ac, which seems odd.
I note that we have an existing similar case with the dual bridge support for the open interface, where we check for feature support with an AC_TRY_COMPILE at configure time, but we always include the related options in the man page.
Would it be OK to tackle the platform-specific man page request in another PR (or do you have some more advice on how I should tackle this here)?
dvanallen
force-pushed
the
add_bind_intf_option
branch
from
1f25acf
to
3740fbc
Compare
dvanallen
force-pushed
the
add_bind_intf_option
branch
from
3740fbc
to
6903862
Compare
This is helpful when using a non-default VRF, because we can pass the VRF interface name to this new option, and then the Linux routing rules for that specific VRF will be used. Some supporting docs are at https://docs.kernel.org/networking/vrf.html, which also notes "Processes can be “VRF aware” by binding a socket to the VRF device."
dvanallen
force-pushed
the
add_bind_intf_option
branch
from
6903862
to
000aba4
Compare
Optionally sets the SO_BINDTODEVICE socket option for LAN connections, to allow binding to a specific interface specified on the command line. This has been tested successfully on bare-metal Linux servers.