Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has submitted this change and it was merged. (
https://gerrit.osmocom.org/12318 )
Change subject: ctrl: log host/port on errors
..
ctrl: log host/port on errors
In case of multiple ctrl-client entries in .cfg file it's impossible to
see which one is causing particular ctrl error. Fix this by introducing
macro wrapper for stderr logging which always show host:port relevant to
the error.
Change-Id: I788d51359965a66c54075a3971aa7824c3bfb0bf
Related: SYS#2655
---
M src/simple_ctrl.c
1 file changed, 24 insertions(+), 16 deletions(-)
Approvals:
Jenkins Builder: Verified
daniel: Looks good to me, approved
osmith: Looks good to me, but someone else must approve
diff --git a/src/simple_ctrl.c b/src/simple_ctrl.c
index 2261323..b56fc83 100644
--- a/src/simple_ctrl.c
+++ b/src/simple_ctrl.c
@@ -37,6 +37,9 @@
#include "simple_ctrl.h"
+#define CTRL_ERR(cfg, fmt, args...) \
+ fprintf(stderr, "CTRL %s:%u error: " fmt, cfg.remote_host,
cfg.remote_port, ##args)
+
/***
* blocking I/O with timeout helpers
***/
@@ -98,20 +101,28 @@
int fd;
uint32_t next_id;
uint32_t tout_msec;
+ struct ctrl_cfg cfg;
};
struct simple_ctrl_handle *simple_ctrl_open(void *ctx, const char *host,
uint16_t dport,
uint32_t tout_msec)
{
- struct simple_ctrl_handle *sch;
+ struct simple_ctrl_handle *sch = talloc_zero(ctx, struct
simple_ctrl_handle);
fd_set writeset;
int off = 0;
int rc, fd;
+ if (!sch)
+ return NULL;
+
+ sch->cfg.name = talloc_strdup(sch, "simple-ctrl");
+ sch->cfg.remote_host = talloc_strdup(sch, host);
+ sch->cfg.remote_port = dport;
+
fd = osmo_sock_init(AF_INET, SOCK_STREAM, IPPROTO_TCP, host, dport,
OSMO_SOCK_F_CONNECT | OSMO_SOCK_F_NONBLOCK);
if (fd < 0) {
- fprintf(stderr, "CTRL: error connecting socket: %s\n",
strerror(errno));
+ CTRL_ERR(sch->cfg, "connecting socket: %s\n", strerror(errno));
return NULL;
}
@@ -120,23 +131,20 @@
FD_SET(fd, );
rc = select(fd+1, NULL, , NULL, timeval_from_msec(tout_msec));
if (rc == 0) {
- fprintf(stderr, "CTRL: timeout during connect\n");
+ CTRL_ERR(sch->cfg, "timeout during connect\n");
goto out_close;
}
if (rc < 0) {
- fprintf(stderr, "CTRL: error connecting socket: %s\n",
strerror(errno));
+ CTRL_ERR(sch->cfg, "error connecting socket: %s\n",
strerror(errno));
goto out_close;
}
/* set FD blocking again */
if (ioctl(fd, FIONBIO, (unsigned char *)) < 0) {
- fprintf(stderr, "CTRL: cannot set socket blocking: %s\n",
strerror(errno));
+ CTRL_ERR(sch->cfg, "cannot set socket blocking: %s\n",
strerror(errno));
goto out_close;
}
- sch = talloc_zero(ctx, struct simple_ctrl_handle);
- if (!sch)
- goto out_close;
sch->fd = fd;
sch->tout_msec = tout_msec;
return sch;
@@ -165,10 +173,10 @@
rc = read_timeout(sch->fd, (uint8_t *) , sizeof(hh), sch->tout_msec);
if (rc < 0) {
- fprintf(stderr, "CTRL: Error during read: %d\n", rc);
+ CTRL_ERR(sch->cfg, "read(): %d\n", rc);
return NULL;
} else if (rc < sizeof(hh)) {
- fprintf(stderr, "CTRL: ERROR: short read (header)\n");
+ CTRL_ERR(sch->cfg, "short read (header)\n");
return NULL;
}
len = ntohs(hh.len);
@@ -182,7 +190,7 @@
resp->l2h = resp->tail;
rc = read(sch->fd, resp->l2h, len);
if (rc < len) {
- fprintf(stderr, "CTRL: ERROR: short read (payload)\n");
+ CTRL_ERR(sch->cfg, "short read (payload)\n");
msgb_free(resp);
return NULL;
}
@@ -214,7 +222,7 @@
*tmp = '\0';
return resp;
} else {
- fprintf(stderr, "unknown IPA message %s\n",
msgb_hexdump(resp));
+ CTRL_ERR(sch->cfg, "unknown IPA message %s\n",
msgb_hexdump(resp));
msgb_free(resp);
}
}
@@ -229,10 +237,10 @@
rc = write_timeout(sch->fd, msg->data, msg->len, sch->tout_msec);
if (rc < 0) {
- fprintf(stderr, "CTRL: Error during write: %d\n", rc);
+ CTRL_ERR(sch->cfg, "write(): %d\n", rc);
return rc;
} else if (rc < msg->len) {
- fprintf(stderr, "CTRL: ERROR: short write\n");
+ CTRL_ERR(sch->cfg, "short
Change in osmo-sysmon[master]: ctrl: log host/port on errors
daniel has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 7: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 7 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: daniel Gerrit-Reviewer: osmith Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Thu, 07 Feb 2019 10:36:55 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-sysmon[master]: ctrl: log host/port on errors
osmith has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 6: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 6 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: daniel Gerrit-Reviewer: osmith Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Tue, 29 Jan 2019 12:39:40 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 6: Both items should be addressed now. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 6 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: daniel Gerrit-Reviewer: osmith Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Tue, 29 Jan 2019 11:17:28 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
osmith has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 5: Code-Review-1 (2 comments) https://gerrit.osmocom.org/#/c/12318/4/src/simple_ctrl.c File src/simple_ctrl.c: https://gerrit.osmocom.org/#/c/12318/4/src/simple_ctrl.c@40 PS4, Line 40: #define CTRL_ERR(cfg, fmt, args...) fprintf(stderr, "CTRL %s:%u error: " fmt, cfg->remote_host, cfg->remote_port, ##args) This line is longer than 120 characters, which is the max allowed length defined here: https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards https://gerrit.osmocom.org/#/c/12318/4/src/simple_ctrl.c@103 PS4, Line 103: struct ctrl_cfg *cfg; Pau asked in your other path: > Do we really need a pointer allocated by talloc here? and an alloc function? > just to have 2 strings and one int, all this seems overengineered to me. https://gerrit.osmocom.org/#/c/osmo-sysmon/+/12316/2/src/osysmon_ctrl.c@41 Then you changed it to "struct ctrl_cfg cfg;". So shouldn't it be the same here for consistency? -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: daniel Gerrit-Reviewer: osmith Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Mon, 28 Jan 2019 15:21:56 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 3: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 3 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Fri, 28 Dec 2018 18:44:07 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/#/c/12318/2/src/osysmon_ctrl.c File src/osysmon_ctrl.c: https://gerrit.osmocom.org/#/c/12318/2/src/osysmon_ctrl.c@25 PS2, Line 25: #include Why is this one needed? Doesn't seem related to the patch. https://gerrit.osmocom.org/#/c/12318/2/src/simple_ctrl.c File src/simple_ctrl.c: https://gerrit.osmocom.org/#/c/12318/2/src/simple_ctrl.c@53 PS2, Line 53: static void ctrl_cfg_printf(const struct ctrl_cfg *cfg, const char *fmt, ...) Having this instead of a macro looks overkill to me, but fine. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-CC: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 15:45:34 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 2: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 15:37:15 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 1: > Patch Set 1: > > > What would be the right approach to make host:port available for logging > > than? > > Perhaps store the information you need to log in struct simple_ctrl_handle? It'll breaks layering as well: we'll either have to duplicate host:port info from anonymous struct inside config and keep it in sync or move this data out of config. Both ways seems worse to me - we either loose single source of truth or we spread config parameters over several structs. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Mon, 17 Dec 2018 12:28:21 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 1: > What would be the right approach to make host:port available for logging than? Perhaps store the information you need to log in struct simple_ctrl_handle? -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Mon, 17 Dec 2018 10:47:01 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 1: > Patch Set 1: Code-Review-1 > > I feel this entire patchset breaks layering. The simple_ctrl_client doesnt > have a configuration. the configuration is part of the main program. but now > you're passing the configuration around everywhere, adding additional > arguments to the functions, making them introspect somethin that's not theirs. What would be the right approach to make host:port available for logging than? -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Comment-Date: Fri, 14 Dec 2018 15:20:39 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12318 ) Change subject: ctrl: log host/port on errors .. Patch Set 1: Code-Review-1 I feel this entire patchset breaks layering. The simple_ctrl_client doesnt have a configuration. the configuration is part of the main program. but now you're passing the configuration around everywhere, adding additional arguments to the functions, making them introspect somethin that's not theirs. -- To view, visit https://gerrit.osmocom.org/12318 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: I788d51359965a66c54075a3971aa7824c3bfb0bf Gerrit-Change-Number: 12318 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Comment-Date: Fri, 14 Dec 2018 15:17:16 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-sysmon[master]: ctrl: log host/port on errors
Max has uploaded this change for review. ( https://gerrit.osmocom.org/12318
Change subject: ctrl: log host/port on errors
..
ctrl: log host/port on errors
In case of multiple ctrl-client entries in .cfg file it's impossible to
see which one is causing particular ctrl error. Fix this by introducing
macro wrapper for stderr logging which always show host:port relevant to
the error.
Change-Id: I788d51359965a66c54075a3971aa7824c3bfb0bf
Related: SYS#2655
---
M src/simple_ctrl.c
1 file changed, 18 insertions(+), 16 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sysmon refs/changes/18/12318/1
diff --git a/src/simple_ctrl.c b/src/simple_ctrl.c
index 24d92b9..433f3bf 100644
--- a/src/simple_ctrl.c
+++ b/src/simple_ctrl.c
@@ -37,6 +37,8 @@
#include "simple_ctrl.h"
+#define CTRL_ERR(c, fmt, args...) fprintf(stderr, "CTRL %s:%u error: " fmt
"\n", c->remote_host, c->remote_port, ##args)
+
/***
* blocking I/O with timeout helpers
***/
@@ -110,7 +112,7 @@
fd = osmo_sock_init(AF_INET, SOCK_STREAM, IPPROTO_TCP,
cfg->remote_host, cfg->remote_port,
OSMO_SOCK_F_CONNECT | OSMO_SOCK_F_NONBLOCK);
if (fd < 0) {
- fprintf(stderr, "CTRL: error connecting socket: %s\n",
strerror(errno));
+ CTRL_ERR(cfg, "connecting socket: %s", strerror(errno));
return NULL;
}
@@ -119,17 +121,17 @@
FD_SET(fd, );
rc = select(fd+1, NULL, , NULL, timeval_from_msec(tout_msec));
if (rc == 0) {
- fprintf(stderr, "CTRL: timeout during connect\n");
+ CTRL_ERR(cfg, "timeout during connect");
goto out_close;
}
if (rc < 0) {
- fprintf(stderr, "CTRL: error connecting socket: %s\n",
strerror(errno));
+ CTRL_ERR(cfg, "error connecting socket: %s", strerror(errno));
goto out_close;
}
/* set FD blocking again */
if (ioctl(fd, FIONBIO, (unsigned char *)) < 0) {
- fprintf(stderr, "CTRL: cannot set socket blocking: %s\n",
strerror(errno));
+ CTRL_ERR(cfg, "cannot set socket blocking: %s",
strerror(errno));
goto out_close;
}
@@ -156,7 +158,7 @@
talloc_free(sch);
}
-static struct msgb *simple_ipa_receive(struct simple_ctrl_handle *sch)
+static struct msgb *simple_ipa_receive(const struct ctrl_cfg *cfg, struct
simple_ctrl_handle *sch)
{
struct ipaccess_head hh;
struct msgb *resp;
@@ -164,10 +166,10 @@
rc = read_timeout(sch->fd, (uint8_t *) , sizeof(hh), sch->tout_msec);
if (rc < 0) {
- fprintf(stderr, "CTRL: Error during read: %d\n", rc);
+ CTRL_ERR(cfg, "read(): %d", rc);
return NULL;
} else if (rc < sizeof(hh)) {
- fprintf(stderr, "CTRL: ERROR: short read (header)\n");
+ CTRL_ERR(cfg, "short read (header)");
return NULL;
}
len = ntohs(hh.len);
@@ -181,7 +183,7 @@
resp->l2h = resp->tail;
rc = read(sch->fd, resp->l2h, len);
if (rc < len) {
- fprintf(stderr, "CTRL: ERROR: short read (payload)\n");
+ CTRL_ERR(cfg, "short read (payload)");
msgb_free(resp);
return NULL;
}
@@ -199,7 +201,7 @@
/* loop until we've received a CTRL message */
while (true) {
- resp = simple_ipa_receive(sch);
+ resp = simple_ipa_receive(cfg, sch);
if (!resp)
return NULL;
@@ -213,13 +215,13 @@
*tmp = '\0';
return resp;
} else {
- fprintf(stderr, "unknown IPA message %s\n",
msgb_hexdump(resp));
+ CTRL_ERR(cfg, "unknown IPA message %s",
msgb_hexdump(resp));
msgb_free(resp);
}
}
}
-static int simple_ctrl_send(struct simple_ctrl_handle *sch, struct msgb *msg)
+static int simple_ctrl_send(const struct ctrl_cfg *cfg, struct
simple_ctrl_handle *sch, struct msgb *msg)
{
int rc;
@@ -228,10 +230,10 @@
rc = write_timeout(sch->fd, msg->data, msg->len, sch->tout_msec);
if (rc < 0) {
- fprintf(stderr, "CTRL: Error during write: %d\n", rc);
+ CTRL_ERR(cfg, "write(): %d", rc);
return rc;
} else if (rc < msg->len) {
- fprintf(stderr, "CTRL: ERROR: short write\n");
+ CTRL_ERR(cfg, "short write");
msgb_free(msg);
return -1;
} else {
@@ -244,7 +246,7 @@
{
int rc;
- rc = simple_ctrl_send(sch, msg);
+ rc = simple_ctrl_send(cfg, sch,
