Re: [PATCH 14/16] gtp-rtnl: Split tid handling for GTPv0 and GTPv1

On Mon, Nov 16, 2015 at 04:06:55PM +0100, Andreas Schultz wrote: 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. [...] [...] [...] [...] [...] So I'd call all of these "tei" and "TEI" (leaving v0 ones called "tid"). "add" to stdout? :) Ah I see, print the usage for the "add" command.... "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", In the GTP headers, there are just three bits available for indicating the GTP version. Any particular reason to pick a uint32_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: [...] > + 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. t is still allocated upon 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); > 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(): [...] > > - 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 possible missing free > + } > > - if (genl_socket_talk(nl, nlh, seq, NULL, NULL) < 0) > - perror("genl_socket_talk"); > + gtp_del_tunnel(genl_id, nl, t); > ...same as above. > struct gtp_pdp { > uint32_t version; > - uint64_t tid; > + struct { > + uint64_t tid; > + } v0; 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? 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