Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12763 )
Change subject: Add OpenVPN probe ...................................................................... Patch Set 5: Code-Review-1 (13 comments) https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c File src/osysmon_openvpn.c: https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@68 PS5, Line 68: get_authority(NULL, vpn->cfg), msgb_length(msg), 128); get_authority memleak, https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@74 PS5, Line 74: fprintf(stderr, "[%s] received message is empty, ignoring...\n", get_authority(NULL, vpn->cfg)); get_authority memleak. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@79 PS5, Line 79: tmp[sizeof(tmp) - 1] = '\0'; if that's a string, then you can simply use osmo_strlcpy afaik. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@82 PS5, Line 82: if (tok) add missing {}, otherwise it's confusing. Only use conditionals without bracket for one-line sections. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@83 PS5, Line 83: switch (i++) { if I understand correctly, first time switch is checked against i=0 (because it's incremented afterwards), and I see no 0 case here. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@127 PS5, Line 127: if (msg == NULL) { (!msg) https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@134 PS5, Line 134: fprintf(stderr, "[%s] unable to receive message in callback\n", get_authority(NULL, vpn->cfg)); mem leak, get_authority allocates stuff. also, msg is leaked. better use goto. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@149 PS5, Line 149: return true; that's weird from API point of view. You usually expect a "create" API to allocate and return newly-allocated struct. Otherwise you never know if a new struct was allocated (which needs to be freed at some point) or an already previously struct was returned (and then avoid double freeing). Calling it "find_or_create" would be more clear. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@159 PS5, Line 159: talloc_free(vpn); Instead of talloc_free + return false in lots of places, use a goto to end of function. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@178 PS5, Line 178: osmo_stream_cli_set_reconnect_timeout(vpn->mgmt, 60); /* FIXME: this seems to be ignored by libosmo-netif */ was this already fixed? otherwise please open a ticket and put the ticket number in the commit for later reference. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@263 PS5, Line 263: value_node_add(vn_host, "remote", remote); possible memleak of remote later on, to be checked. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@267 PS5, Line 267: msgb_printf(msg, "state\n"); Not sure what is this for. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@287 PS5, Line 287: if (llist_count(&g_oss->openvpn_clients)) { No need to count, just check if the list is not empty. -- To view, visit https://gerrit.osmocom.org/12763 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sysmon Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4493e19b9a09dcebd289457eacd1719f7f8cc31c Gerrit-Change-Number: 12763 Gerrit-PatchSet: 5 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Fri, 01 Feb 2019 16:34:56 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
