Change in libosmocore[master]: Introduce tundev API - gerrit-log

23 Jan 2023

Attention is currently required from: osmith, laforge.
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/31025 )

Change subject: Introduce tundev API
......................................................................

Patch Set 11:

(10 comments)

File include/osmocom/core/tun.h:

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/7f40d1e2_79c58aba 
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  […] Ack

File src/core/tun.c:

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/983491c8_0cbf25d9 
PS4, Line 421: osmo_tundev_encaps
...
  agreed Ack

File src/core/tun.c:

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/af32b144_9e7c3eb7 
PS9, Line 130: "
...
  Add "Prepare netns: " for consistency with
other messages in this function? Ack

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/4bb8814e_2817ac8d 
PS9, Line 184: NULL
...
  use a global talloc ctx and pass that here? I
don't plan to pass this from the API, since the ctx passed there is per tundev and may
be expected to be freed when the parent object in the app holding the tudnev is released.
We could add an internal static talloc ctx here if needed, but I don't see urgent need
for it?

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/1c903b80_6b7d7fb1 
PS9, Line 233: inreface
...
  interface Ack

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/fbe9b047_e0eb751e 
PS9, Line 284: tundev_mnl_link_state_change
...
  Maybe call it
"tundev_mnl_link_on_state_change" (adding _on_)? […] Ack

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/8a34baba_4e28637f 
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 br […] Yes this is expected, as per
https://git.netfilter.org/libmnl/tree/examples/rtnl/rtnl-link-set.c

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/32b65316_5fab012c 
PS9, Line 375:  /* RTM_NEWLINK doesn't work either */
...
  leftover comment? Ack

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/409a82f6_83285fbe 
PS9, Line 448: status
...
  why status and not len? Ack

https://gerrit.osmocom.org/c/libosmocore/+/31025/comment/504182d4_eec11582 
PS9, Line 524: free
...
  open Ack

-- 
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: 11
Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt;
Gerrit-Attention: osmith &lt;osmith(a)sysmocom.de&gt;
Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Comment-Date: Mon, 23 Jan 2023 16:40:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith &lt;osmith(a)sysmocom.de&gt;
Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-MessageType: comment