Max has posted comments on this change. ( https://gerrit.osmocom.org/12763 )
Change subject: Add OpenVPN probe ...................................................................... Patch Set 7: (13 comments) This change is ready for review. 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 memleak, Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@74 PS5, Line 74: return NULL; > get_authority memleak. Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@79 PS5, Line 79: > if that's a string, then you can simply use osmo_strlcpy afaik. Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@82 PS5, Line 82: for (tok = strtok(tmp, ","); tok && i < MAX_RESP_COMPONENTS; tok = strtok(NULL, ",")) { > add missing {}, otherwise it's confusing. […] Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@83 PS5, Line 83: /* The string format is documented in https://openvpn.net/community-resources/management-interface/ */ > if I understand correctly, first time switch is checked against i=0 (because > it's incremented afterw […] Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@127 PS5, Line 127: { > (!msg) Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@134 PS5, Line 134: } > mem leak, get_authority allocates stuff. […] Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@149 PS5, Line 149: struct openvpn_client *vpn = openvpn_client_find_or_make(os, host, port); > that's weird from API point of view. […] Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@159 PS5, Line 159: vpn->cfg = host_cfg_alloc(vpn, name, host, port); > Instead of talloc_free + return false in lots of places, use a goto to end of > function. Done https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@178 PS5, Line 178: if (osmo_stream_cli_open(vpn->mgmt) < 0) { > was this already fixed? otherwise please open a ticket and put the ticket > number in the commit for l […] libosmo-netif patches are under review ATM https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@263 PS5, Line 263: > possible memleak of remote later on, to be checked. What makes you think that? It should be automatically cleaned by talloc on vty refresh. https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@267 PS5, Line 267: osmo_stream_cli_send(vpn->mgmt, msg); > Not sure what is this for. Not what you're referring to: - msgb_printf is to print stuff into msgb, documented in libosmocore - osmo_stream_cli_send is to send msgb, documented in libosmo-netif https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@287 PS5, Line 287: struct value_node *vn_vpn = value_node_add(parent, "OpenVPN", NULL); > No need to count, just check if the list is not empty. Done -- 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: 7 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: Thu, 07 Feb 2019 16:45:30 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
