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

[Max]

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 <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: Thu, 07 Feb 2019 16:45:30 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190207/535380c9/attachment.htm>