Patch Set 2: (5 comments)
https://gerrit.osmocom.org/#/c/5216/2/ggsn/ggsn.c File ggsn/ggsn.c: Line 350: if (pdp->peer[1]) { /* ipv4v6 */ > I think a for-loop iterating over the pdp->peer array would look better tha They are not exactly identical because in the first one if it's NULL it should be printed as an error, while on the 2nd one it's fine if there's no peer, because for ipv4 and ipv6 cases it's expected to be NULL. But I'm fine moving it to a loop. PS2, Line 559: sizeof(addr[0])*2 > why not simply sizeof(addr) ? it will clear the entire 'addr' and there's n At the time I was writing the code I was not 100% sure of the output of sizeof(addr), so I went for the secure way :) I'll change it. Line 568: for (i = 0; i < num_addr; i++) { > nothing here ensures that num_addr is actually <=2, right? I would conside I think I'm doing exactly that in all the code from this patch: - documentation from in46a_from_eua states is returns <=2 addresses: "\param[out] dst Array containing 2 in46_addr". It is also expected that returned number is never more than 2. I can detail a bit more the documentation. - In other parts of the code I don't make assumptions, instead I use the "num_addr" variable returned from the function. - I am not making any ssumption on the order of the addresses afair. https://gerrit.osmocom.org/#/c/5216/2/ggsn/icmpv6.c File ggsn/icmpv6.c: Line 196: member = pdp->peer[1]; > we blindly assume that peer[1] is IPv6 type? I mean yes, currently in46a_to Well, I think there's an extra restriction coming from in46a_from_eua: If two IP addresses are returned (function returns 2), then one of the IP addresses is an IPv4 and the other one is an IPv6. Following this restriction, what we do here is check if the first one is IPv4, then we can assume 2nd one is the IPv6. It don't assume here though that the IPv6 is the 2nd one. https://gerrit.osmocom.org/#/c/5216/2/lib/in46_addr.h File lib/in46_addr.h: Line 32: int in46a_to_eua(const struct in46_addr *src, unsigned int size, struct ul66_t *eua); > might be nicer to have a separate function for converting multiple addresse Why do you want to keep the old one? I mean, it lacks features. For API backward compatibility purposes? I think this code is only used in osmo-ggsn binary and it's statically linked, so there's no use for it as far as I understand. -- To view, visit https://gerrit.osmocom.org/5216 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic820759167fd3bdf329cb11d4b942e903fe50af5 Gerrit-PatchSet: 2 Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
