This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Neels Hofmeyr nhofmeyr at sysmocom.deOn Mon, Nov 16, 2015 at 04:06:55PM +0100, Andreas Schultz wrote:
> GTPv1 tunnel use a separate 32bit TIDs for each direction while GTPv0 uses
> only one 64bit TID.
English nitpicking: "GTPv1 tunnels use separate..."
...and the names differ. In v0, it's called "Tunnel IDentifier"; in v1
they're two "Tunnel Endpoint Identifier"s ("TID" vs. "TEI"). This naming
is used consistently in the specs.
> --- a/libgtnl/include/libgtpnl/gtp.h
> +++ b/libgtnl/include/libgtpnl/gtp.h
> @@ -14,6 +14,8 @@ void gtp_tunnel_set_ms_ip4(struct gtp_tunnel *t, struct in_addr *ms_addr);
> void gtp_tunnel_set_sgsn_ip4(struct gtp_tunnel *t, struct in_addr *sgsn_addr);
> void gtp_tunnel_set_version(struct gtp_tunnel *t, uint32_t version);
> void gtp_tunnel_set_tid(struct gtp_tunnel *t, uint64_t tid);
> +void gtp_tunnel_set_i_tid(struct gtp_tunnel *t, uint32_t i_tid);
> +void gtp_tunnel_set_o_tid(struct gtp_tunnel *t, uint32_t o_tid);
[...]
> +uint32_t gtp_tunnel_get_i_tid(struct gtp_tunnel *t);
> +uint32_t gtp_tunnel_get_o_tid(struct gtp_tunnel *t);
[...]
> + GTPA_I_TID,
> + GTPA_O_TID,
[...]
> + union {
[...]
> + struct {
> + uint32_t i_tid;
> + uint32_t o_tid;
> + } v1;
> + } u;
[...]
> + else if (pdp.version == GTP_V1)
> + printf("tid %u/%u ms_addr %s ", pdp.u.v1.i_tid, pdp.u.v1.o_tid, inet_ntoa(pdp.sgsn_addr));
So I'd call all of these "tei" and "TEI" (leaving v0 ones called "tid").
> +static void add_usage(const char *name)
"add" to stdout? :)
Ah I see, print the usage for the "add" command....
> +{
> + printf("%s add <gtp device> <v0> <tid> <ms-addr> <sgsn-addr>\n",
> + name);
> + printf("%s add <gtp device> <v1> <i_tid> <o_tid> <ms-addr> <sgsn-addr>\n",
> + name);
> +}
"v0" and "v1" are the exact literal arguments to pass, in which case I
would omit the braces, like
printf("%s add <gtp device> v1 <i_tid> <o_tid> <ms-addr> <sgsn-addr>\n",
> + uint32_t gtp_version;
In the GTP headers, there are just three bits available for indicating the
GTP version. Any particular reason to pick a uint32_t?
> + t = gtp_tunnel_alloc();
[...]
> + gtp_tunnel_free(t);
It seems that you calloc and free a struct gtp_tunnel within the same
function (I think twice). You could use a local struct instead?
struct gtp_tunnel = {0};
And a possible mem leak because of that:
> static int
> add_tunnel(int argc, char *argv[], int genl_id, struct mnl_socket *nl)
> {
> + struct gtp_tunnel *t;
[...]
> + t = gtp_tunnel_alloc();
> + optidx = 2;
> +
> + gtp_ifidx = if_nametoindex(argv[optidx]);
> if (gtp_ifidx == 0) {
> - fprintf(stderr, "wrong GTP interface %s\n", argv[2]);
> + fprintf(stderr, "wrong GTP interface %s\n", argv[optidx]);
> return EXIT_FAILURE;
t is still allocated upon EXIT_FAILURE.
> }
> + gtp_tunnel_set_ifidx(t, gtp_ifidx);
>
> - if (inet_aton(argv[5], &ms) < 0) {
> - perror("bad address for ms");
> - exit(EXIT_FAILURE);
> - }
> -
> - if (inet_aton(argv[6], &sgsn) < 0) {
> - perror("bad address for sgsn");
> - exit(EXIT_FAILURE);
> - }
> + optidx++;
>
> - if (strcmp(argv[3], "v0") == 0)
> + if (strcmp(argv[optidx], "v0") == 0)
> gtp_version = GTP_V0;
> - else if (strcmp(argv[3], "v1") == 0)
> + else if (strcmp(argv[optidx], "v1") == 0)
> gtp_version = GTP_V1;
> else {
> fprintf(stderr, "wrong GTP version %s, use v0 or v1\n",
> - argv[3]);
> + argv[optidx]);
> return EXIT_FAILURE;
t is still allocated upon EXIT_FAILURE.
> }
> + gtp_tunnel_set_version(t, gtp_version);
>
> - nlh = genl_nlmsg_build_hdr(buf, genl_id, NLM_F_EXCL | NLM_F_ACK, ++seq,
> - GTP_CMD_TUNNEL_NEW);
> - gtp_build_payload(nlh, atoi(argv[4]), gtp_ifidx, sgsn.s_addr,
> - ms.s_addr, gtp_version);
> + if (gtp_version == GTP_V0)
> + gtp_tunnel_set_tid(t, atoi(argv[optidx++]));
> + else if (gtp_version == GTP_V1) {
> + gtp_tunnel_set_i_tid(t, atoi(argv[optidx++]));
> + gtp_tunnel_set_o_tid(t, atoi(argv[optidx++]));
> + }
>
> - if (genl_socket_talk(nl, nlh, seq, NULL, NULL) < 0)
> - perror("genl_socket_talk");
> + if (inet_aton(argv[optidx++], &ms) < 0) {
> + perror("bad address for ms");
> + exit(EXIT_FAILURE);
Some failures return EXIT_FAILURE, some exit() directly? Is that intended?
> + }
> + gtp_tunnel_set_ms_ip4(t, &ms);
> +
> + if (inet_aton(argv[optidx++], &sgsn) < 0) {
> + perror("bad address for sgsn");
> + exit(EXIT_FAILURE);
> + }
> + gtp_tunnel_set_sgsn_ip4(t, &sgsn);
> +
> + gtp_add_tunnel(genl_id, nl, t);
>
> + gtp_tunnel_free(t);
Just to make sure ... t's data is copied by gtp_add_tunnel(), right?
All in all, seems to be a case for a local struct.
Same thing in del_tunnel():
> return 0;
> }
>
> static int
> del_tunnel(int argc, char *argv[], int genl_id, struct mnl_socket *nl)
> {
> + struct gtp_tunnel *t;
[...]
> + t = gtp_tunnel_alloc();
>
> - nlh = genl_nlmsg_build_hdr(buf, genl_id, NLM_F_ACK, ++seq,
> - GTP_CMD_TUNNEL_DELETE);
> - gtp_build_payload(nlh, atoi(argv[4]), gtp_ifidx, 0, 0, atoi(argv[3]));
> + gtp_ifidx = if_nametoindex(argv[2]);
> + if (gtp_ifidx == 0) {
> + fprintf(stderr, "wrong GTP interface %s\n", argv[2]);
> + return EXIT_FAILURE;
possible missing free
> + }
> + gtp_tunnel_set_ifidx(t, gtp_ifidx);
> +
> + if (strcmp(argv[3], "v0") == 0) {
> + gtp_tunnel_set_version(t, GTP_V0);
> + gtp_tunnel_set_tid(t, atoi(argv[4]));
> + } else if (strcmp(argv[3], "v1") == 0) {
> + gtp_tunnel_set_version(t, GTP_V1);
> + gtp_tunnel_set_i_tid(t, atoi(argv[4]));
> + } else {
> + fprintf(stderr, "wrong GTP version %s, use v0 or v1\n",
> + argv[3]);
> + return EXIT_FAILURE;
possible missing free
> + }
>
> - if (genl_socket_talk(nl, nlh, seq, NULL, NULL) < 0)
> - perror("genl_socket_talk");
> + gtp_del_tunnel(genl_id, nl, t);
>
> + gtp_tunnel_free(t);
...same as above.
> struct gtp_pdp {
> uint32_t version;
> - uint64_t tid;
> + union {
> + struct {
> + uint64_t tid;
> + } v0;
> + struct {
> + uint32_t i_tid;
> + uint32_t o_tid;
> + } v1;
> + } u;
The same union again? Actually, it seems the entire struct gtp_pdp
definition is duplicated. Wouldn't it be better to define it once and
reuse?
> @@ -152,12 +197,16 @@ static int genl_gtp_validate_cb(const struct nlattr *attr, void *data)
> static int genl_gtp_attr_cb(const struct nlmsghdr *nlh, void *data)
> {
> struct nlattr *tb[GTPA_MAX + 1] = {};
> - struct gtp_pdp pdp;
> + struct gtp_pdp pdp = {};
Heh, exactly :)
Actually, we've recently had a discussion in the sysmocom office about
this way of zero initialization. There's the "traditional" method of
naming one element as zero, after which the remaining elements will also
be initialized to nul. If the first member is a primitive, {0} suffices.
If not, one has to explicitly name a member, which can be cumbersome. Then
I came across the possibility to just omit all members and obviously
preferred that: {}. However, that doesn't seem to be part of C99, so now
I'm back to naming a member again. Any further insights on the subject
would be appreciated...
~Neels
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20151119/9966a5a0/attachment.bin>