Introduce a netlink alloc driver . #1981
Conversation
base/cvd/allocd/alloc_netlink.cpp
Outdated
| std::string n = std::string(name); | ||
| cuttlefish::NetlinkRequest req(RTM_NEWLINK, NLM_F_REQUEST | NLM_F_ACK); | ||
| req.AddIfInfo(0, true); | ||
| req.AddString(IFLA_IFNAME, n); |
There was a problem hiding this comment.
NetlinkRequest::AddString could be modified to accept a std::string_view as it doesn't actually depend on null termination and uses the length.
There was a problem hiding this comment.
Yes, though I don't know if netlink itself will tolerate a null-terminated string. I could go looking, but it might be better to separate that in another commit.
base/cvd/allocd/alloc_netlink.cpp
Outdated
| int RunExternalCommand(const std::string& command) { | ||
| FILE* fp; | ||
| LOG(INFO) << "Running external command: " << command; | ||
| fp = popen(command.c_str(), "r"); | ||
|
|
||
| if (fp == nullptr) { | ||
| LOG(WARNING) << "Error running external command"; | ||
| return -1; | ||
| } | ||
|
|
||
| int status = pclose(fp); | ||
| int ret = -1; | ||
| if (status == -1) { | ||
| LOG(WARNING) << "pclose error"; | ||
| } else { | ||
| if (WIFEXITED(status)) { | ||
| LOG(INFO) << "child process exited normally"; | ||
| ret = WEXITSTATUS(status); | ||
| } else if (WIFSIGNALED(status)) { | ||
| int sig = WTERMSIG(status); | ||
| LOG(WARNING) << "child process was terminated by signal " | ||
| << strsignal(sig) << " (" << sig << ")"; | ||
| } else { | ||
| LOG(WARNING) << "child process did not terminate normally"; | ||
| } | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Can RunExternalCommand be in a separate file so it can be shared with alloc_iproute2?
There was a problem hiding this comment.
I'll put this back in alloc_utils and add extern decls to the drivers. We should probably kill it soon and use the existing -- I think it's called Command? -- boilerplate for this.
There was a problem hiding this comment.
Yeah there's Command though the closest immediate equivalent is Execute in that file.
The main behavioral difference is that Command and Execute are direct wrappers over execve while popen is a wrapper over the shell, effectively popen(cmd) is equivalent to Execute({"/bin/sh", cmd}). Not a big deal since the one usage here doesn't use pipe redirection or other shell features. The only thing to watch out for is that arguments should be separated into multiple members, "-t nat" will probably do the wrong thing and should be "-t", "nat" for a direct invocation of iptables.
base/cvd/allocd/alloc_netlink.cpp
Outdated
|
|
||
| unsigned int Index(std::string_view ifname) { | ||
| std::string name = std::string(ifname); | ||
| return if_nametoindex(name.c_str()); |
There was a problem hiding this comment.
Can this handle this error case of if_nametoindex?
on error, 0 is returned and
errnois set to indicate the error.
There was a problem hiding this comment.
Done. Again, this required a PCHECK, but since we want somewhat more graceful handling when we're tearing down resources (e.g., in error cases), we have the unchecked if_nametoindex in those cases.
There was a problem hiding this comment.
It could also return the Result type, which lets the caller decide how to handle failures, by continuing or letting the failures propagate upwards.
3405691582
force-pushed
the
netlink_4
branch
3 times, most recently
from
54700ed to
91742de
Compare
We're now in the position to introduce a netlink driver. This driver allows cvdalloc to (transitively) set up nearly all of the network allocation and deallocation steps in a faster, more idiomatic way than shelling out to ip(8). Only one driver operation still requires shelling out to iptables(8). We can potentially use NETLINK_NETFILTER to do this in the future but this is actually somewhat more painful and involved than using netlink for the other driver operations. Maybe we can do this in the future; left as a TODO for now. To use the driver until the default is changed, one can rebuild cvd with the `--//allocd:driver=netlink` custom bazel flag; without the flag, the the iproute2 driver is the default, for reasons mentioned in earlier commits.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| #include "alloc_driver.h" |
There was a problem hiding this comment.
nit: please use a path relative to the bazel workspace root.
base/cvd/allocd/alloc_netlink.cpp
Outdated
|
|
||
| unsigned int Index(std::string_view ifname) { | ||
| std::string name = std::string(ifname); | ||
| return if_nametoindex(name.c_str()); |
There was a problem hiding this comment.
It could also return the Result type, which lets the caller decide how to handle failures, by continuing or letting the failures propagate upwards.
2 participants
We're now in the position to introduce a netlink driver. This driver allows cvdalloc to (transitively) set up nearly all of the network allocation and deallocation steps in a faster, more idiomatic way than shelling out to ip(8).
Only one driver operation still requires shelling out to iptables(8). We can potentially use NETLINK_NETFILTER to do this in the future but this is actually somewhat more painful and involved than using netlink for the other driver operations. Maybe we can do this in the future; left as a TODO for now.
To use the driver until the default is changed, one can rebuild cvd with the
--//allocd:driver=netlinkcustom bazel flag; without the flag, the the iproute2 driver is the default, for reasons mentioned in earlier commits.