Attention is currently required from: laforge, pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31025 ) Change subject: Introduce tundev API ...................................................................... Patch Set 9: (12 comments) File include/osmocom/core/tun.h: https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/032d2267_25b699d0 PS9, Line 29: const char *osmo_tundev_get_name(const struct osmo_tundev *tundev); maybe add a new line below this? given that the two functions below are about dev_name, and there's another line break before netns_name File src/core/tun.c: https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/321c2301_3ec32660 PS4, Line 238: LOGTUN(tundev, LOGL_ERROR, "Cannot obtain netns file descriptor: %s (%d)\n", (not relevant anymore in current patchset) https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/7fee7afd_ed055859 PS4, Line 245: if (tundev->netns_name) { Done https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/8e9bd2a8_14221f54 PS4, Line 405: whien Done File src/core/tun.c: https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/aee4e42a_72ad18d0 PS9, Line 130: " Add "Prepare netns: " for consistency with other messages in this function? https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/5248d136_b8fee7b4 PS9, Line 184: NULL use a global talloc ctx and pass that here? https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/9bd76b93_bd734cd9 PS9, Line 233: inreface interface https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/2c704af6_a968d916 PS9, Line 284: tundev_mnl_link_state_change Maybe call it "tundev_mnl_link_on_state_change" (adding _on_)? I just read through it and was confused that there was no api call to libmnl to actually change the state before realizing that this gets called by the function below after it was changed, not to change it. https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/98fec7d2_a95c2ce0 PS9, Line 371: |= is this intentionally the same as in the if (up) case? if so, why have it in both the if and else branches? https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/596a974e_23640af6 PS9, Line 375: /* RTM_NEWLINK doesn't work either */ leftover comment? https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/4a184604_be13ca90 PS9, Line 448: status why status and not len? https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/8ab26164_c33fee79 PS9, Line 524: free open -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31025 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I3463271666df1e85746fb7b06ec45a17024b8c53 Gerrit-Change-Number: 31025 Gerrit-PatchSet: 9 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 23 Jan 2023 15:46:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith <osmith(a)sysmocom.de> Gerrit-MessageType: comment