Change in osmo-sysmon[master]: Add OpenVPN probe

[Pau Espin Pedrol]

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 <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 01 Feb 2019 16:34:56 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190201/d8362a25/attachment.htm>