Hi Alvaro,
Several comments below.
On Tue, Apr 08, 2014 at 02:31:33PM +0200, Alvaro Neira Ayuso wrote:
From: Álvaro Neira Ayuso <anayuso(a)sysmocom.de>
With this patch, the manager monitors the sensors and send
OML Failure message from the Manager to the BTS and the BTS
to BSC, for having a report system of the sensors.
Signed-off-by: Alvaro Neira Ayuso <anayuso(a)sysmocom.de>
---
v5: Changed the osmo_fd name variables for using anothers more clear. Used enum
for declaring states in the connections. Refactored some code for doing some
functions for having a code more short and clear. Changed the function
connect for only using the timer in the case that the socket is disconnected.
Changed some fails when i have used strncpy. And changed some coding style
fails.
Please, use a item list next time. It's a bit more readable.
src/osmo-bts-sysmo/main.c | 115
+++++++++++++++++++++++++
src/osmo-bts-sysmo/misc/sysmobts_mgr.c | 90 ++++++++++++++++++++
src/osmo-bts-sysmo/misc/sysmobts_misc.c | 142 ++++++++++++++++++++++++++++++-
src/osmo-bts-sysmo/misc/sysmobts_misc.h | 33 +++++++
4 files changed, 379 insertions(+), 1 deletion(-)
diff --git a/src/osmo-bts-sysmo/main.c b/src/osmo-bts-sysmo/main.c
index 74ee47f..61a5716 100644
--- a/src/osmo-bts-sysmo/main.c
+++ b/src/osmo-bts-sysmo/main.c
@@ -35,8 +35,10 @@
#include <osmocom/core/talloc.h>
#include <osmocom/core/application.h>
+#include <osmocom/core/socket.h>
#include <osmocom/vty/telnet_interface.h>
#include <osmocom/vty/logging.h>
+#include <osmocom/gsm/protocol/ipaccess.h>
#include <osmo-bts/gsm_data.h>
#include <osmo-bts/logging.h>
@@ -45,13 +47,19 @@
#include <osmo-bts/vty.h>
#include <osmo-bts/bts_model.h>
#include <osmo-bts/pcu_if.h>
+#include <osmo-bts/oml.h>
#define SYSMOBTS_RF_LOCK_PATH "/var/lock/bts_rf_lock"
+#define SOCKET_PATH "/var/run/bts_oml"
^^^^^
I can see spaces here, this should be indented with tabs.
+
+#define IPA_HEADER_SIZE 3
Please, better use sizeof(struct ipaccess_head) instead of this
constant.
+#define IPA_OML_PROTO 0xFF
#include "utils.h"
#include "eeprom.h"
#include "l1_if.h"
#include "hw_misc.h"
+#include "btsconfig.h"
/* FIXME: read from real hardware */
const uint8_t abis_mac[6] = { 0,1,2,3,4,5 };
@@ -258,6 +266,7 @@ static void signal_handler(int signal)
case SIGINT:
//osmo_signal_dispatch(SS_GLOBAL, S_GLOBAL_SHUTDOWN, NULL);
bts_shutdown(bts, "SIGINT");
+ unlink(SOCKET_PATH);
break;
case SIGABRT:
case SIGUSR1:
@@ -287,6 +296,95 @@ static int write_pid_file(char *procname)
return 0;
}
+#ifdef BUILD_SBTS2050
+static int test_recv_msg(struct msgb *msg)
+{
+ struct ipaccess_head *hh = (struct ipaccess_head *)msg->data;
+ struct abis_om_hdr *omh;
Insufficiente sanity checks here:
if (msg->len < sizeof(struct ipaccess_head))
return -1;
Now you can safely access the content of the ipa header.
hh = (struct ipaccess_head *)msg->data;
+ if (hh->proto != IPA_OML_PROTO)
+ return -1;
+
+ if (ntohs(hh->len) != msg->len)
+ return -1;
+
+ msgb_pull(msg, IPA_HEADER_SIZE);
OK, now you can pull the ipa header.
+
+ msg->l2h = msg->data;
+ msg->l3h = msg->data + sizeof(struct abis_om_hdr);
Now you have to sanity check the abis OM header before you can access
it, is there room for this?
+ omh = (struct abis_om_hdr *) msg->l2h;
+
+ if (omh->mdisc != ABIS_OM_MDISC_FOM)
+ return -1;
+
+ if (omh->placement != ABIS_OM_PLACEMENT_ONLY)
+ return -1;
+
+ if (omh->sequence != 0)
+ return -1;
+
+ if (omh->length != sizeof(struct abis_om_fom_hdr))
+ return -1;
Use logging to inform about malformed messages?
+
+ return 0;
+}
+
+static int read_sock(struct osmo_fd *fd, unsigned int what)
+{
+ struct msgb *msg;
+ struct gsm_abis_mo *mo;
+ int rc;
+
+ msg = oml_msgb_alloc();
+ if (msg == NULL)
+ return -1;
+
+ rc = recv(fd->fd, msg->tail, msg->data_len, 0);
+ if (rc <= 0) {
+ close(fd->fd);
+ osmo_fd_unregister(fd);
+ fd->fd = -1;
You're leaking msg.
msgb_free(msg)
+ return -1;
+ }
+
+ msgb_put(msg, rc);
+
+ if (test_recv_msg(msg) < 0) {
+ LOGP(DL1C, LOGL_NOTICE, "Failed: Malformed receive message");
Same here, please release the msg.
+ return -1;
+ }
+
+ mo = &bts->mo;
+ msg->trx = mo->bts->c0;
+
+ return abis_oml_sendmsg(msg);
+}
+
+static int accept_unix_sock(struct osmo_fd *fd, unsigned int what)
+{
+ int sfd = fd->fd, cl;
+ struct osmo_fd *read_fd = (struct osmo_fd *)fd->data;
+
+ if (read_fd->fd > -1) {
+ close(read_fd->fd);
+ osmo_fd_unregister(read_fd);
+ read_fd->fd = -1;
+ }
+
+ cl = accept(sfd, NULL, NULL);
+ if (cl < 0)
+ return -1;
+
+ read_fd->fd = cl;
+ if (osmo_fd_register(read_fd) != 0) {
+ close(cl);
+ return -1;
+ }
+
+ return 0;
+}
+#endif
int main(int argc, char **argv)
{
@@ -295,6 +393,9 @@ int main(int argc, char **argv)
struct gsm_bts_role_bts *btsb;
struct e1inp_line *line;
void *tall_msgb_ctx;
+#ifdef BUILD_SBTS2050
+ struct osmo_fd accept_fd, read_fd;
+#endif
int rc;
tall_bts_ctx = talloc_named_const(NULL, 1, "OsmoBTS context");
@@ -370,6 +471,20 @@ int main(int argc, char **argv)
fprintf(stderr, "unable to connect to BSC\n");
exit(1);
}
+#ifdef BUILD_SBTS2050
+ accept_fd.cb = accept_unix_sock;
+ read_fd.cb = read_sock;
+ read_fd.when = BSC_FD_READ;
+ read_fd.fd = -1;
+ accept_fd.data = &read_fd;
+
+ rc = osmo_sock_unix_init_ofd(&accept_fd, SOCK_SEQPACKET, 0, SOCKET_PATH,
+ OSMO_SOCK_F_BIND | OSMO_SOCK_F_NONBLOCK);
+ if (rc < 0) {
+ perror("Error creating socket domain creation");
+ exit(3);
+ }
+#endif
You can probably encapsulate this in a function:
rc = sbts2050_sock_unix_init(...);
Then in this function you can do:
int sbts2050_sock_unix_init(...)
{
#ifdef BUILD_SBTS2050
...
#else
return 0;
#endif
}
if (daemonize) {
rc = osmo_daemonize();
diff --git a/src/osmo-bts-sysmo/misc/sysmobts_mgr.c
b/src/osmo-bts-sysmo/misc/sysmobts_mgr.c
index 6c64d0f..b8813f0 100644
--- a/src/osmo-bts-sysmo/misc/sysmobts_mgr.c
+++ b/src/osmo-bts-sysmo/misc/sysmobts_mgr.c
@@ -36,6 +36,7 @@
#include <osmocom/core/timer.h>
#include <osmocom/core/msgb.h>
#include <osmocom/core/serial.h>
+#include <osmocom/core/socket.h>
#include <osmocom/vty/telnet_interface.h>
#include <osmocom/vty/logging.h>
@@ -45,9 +46,16 @@
#include "misc/sysmobts_nl.h"
#include "misc/sysmobts_par.h"
+enum {
+ SYSMO_MGR_DISCONNECTED = 0,
+ SYSMO_MGR_CONNECTED,
+};
+
static int no_eeprom_write = 0;
static int daemonize = 0;
+static int fd_unix = -1;
void *tall_mgr_ctx;
+static struct sbts2050_config_info confinfo;
/* every 6 hours means 365*4 = 1460 EEprom writes per year (max) */
#define TEMP_TIMER_SECS (6 * 3600)
@@ -55,7 +63,63 @@ void *tall_mgr_ctx;
/* every 1 hours means 365*24 = 8760 EEprom writes per year (max) */
#define HOURS_TIMER_SECS (1 * 3600)
+/* every 5 minutes try to reconnect if we have a problem in the communication*/
+#define CONNECT_TIMER_SECS 300
This is quite a lot, you should retry a bit more often, at least every
30 seconds I'd say.
+/* unix domain socket file descriptor */
+#define SOCKET_PATH "/var/run/bts_oml"
+
#ifdef BUILD_SBTS2050
+static int trx_nr = -1;
+static int state_connection = 0;
+
+static struct osmo_timer_list connect_timer;
+static void socket_connect_cb(void *data)
+{
+ if (state_connection == SYSMO_MGR_DISCONNECTED) {
Is it possible to run socket_connect_cb in connected state? I don't
think so, so you can remove this if branch.
+ fd_unix = osmo_sock_unix_init(SOCK_SEQPACKET, 0,
SOCKET_PATH,
+ OSMO_SOCK_F_CONNECT);
+ if (fd_unix < 0) {
+ osmo_timer_schedule(&connect_timer,
+ CONNECT_TIMER_SECS, 0);
+ return;
+ }
+
+ state_connection = SYSMO_MGR_CONNECTED;
+ }
+}
+
+static void check_temperature(struct uc *ucontrol0, int downlimit, int uplimit,
+ int current_temp,
+ enum sbts2050_temp_sensor sensor,
+ enum sbts2050_alert_lvl alert)
+{
+ int rc;
+
+ if (downlimit >= current_temp || uplimit <= current_temp) {
+ switch (alert) {
+ case SBTS2050_WARN_ALERT:
+ rc = sendto_osmobts(fd_unix, ucontrol0, alert, sensor,
+ &confinfo, trx_nr);
+ break;
+ case SBTS2050_SEVER_ALERT:
+ rc = sendto_osmobts(fd_unix, ucontrol0, alert, sensor,
+ &confinfo, trx_nr);
+ sbts2050_uc_power(ucontrol0, confinfo.master_power_act,
+ confinfo.slave_power_act,
+ confinfo.pa_power_act);
+ break;
+ }
+ }
+
+ state_connection = rc;
+
+ if (state_connection == SYSMO_MGR_DISCONNECTED) {
+ close(fd_unix);
+ socket_connect_cb(NULL);
+ }
+}
+
static struct osmo_timer_list temp_uc_timer;
static void check_uctemp_timer_cb(void *data)
{
@@ -64,6 +128,26 @@ static void check_uctemp_timer_cb(void *data)
sbts2050_uc_check_temp(ucontrol0, &temp_pa, &temp_board);
+ confinfo.temp_pa_cur = temp_pa;
+ confinfo.temp_board_cur = temp_board;
+
+ check_temperature(ucontrol0, confinfo.temp_min_pa_warn_limit,
+ confinfo.temp_max_pa_warn_limit,
+ temp_pa, SBTS2050_TEMP_PA, SBTS2050_WARN_ALERT);
+
+ check_temperature(ucontrol0, confinfo.temp_min_pa_sever_limit,
+ confinfo.temp_max_pa_sever_limit,
+ temp_pa, SBTS2050_TEMP_PA, SBTS2050_SEVER_ALERT);
+
+ check_temperature(ucontrol0, confinfo.temp_min_board_warn_limit,
+ confinfo.temp_max_board_warn_limit,
+ temp_board, SBTS2050_TEMP_BOARD, SBTS2050_WARN_ALERT);
+
+ check_temperature(ucontrol0, confinfo.temp_min_board_sever_limit,
+ confinfo.temp_min_board_sever_limit,
+ temp_board, SBTS2050_TEMP_BOARD,
+ SBTS2050_SEVER_ALERT);
+
osmo_timer_schedule(&temp_uc_timer, TEMP_TIMER_SECS, 0);
}
#endif
@@ -93,6 +177,7 @@ static void initialize_sbts2050(void)
if (val != 0)
return;
}
+ trx_nr = val;
ucontrol0.fd = osmo_serial_init(ucontrol0.path, 115200);
if (ucontrol0.fd < 0) {
@@ -101,6 +186,10 @@ static void initialize_sbts2050(void)
return;
}
+ /* start handle for reconnect the socket in case of error */
+ connect_timer.cb = socket_connect_cb;
+ socket_connect_cb(NULL);
+
temp_uc_timer.cb = check_uctemp_timer_cb;
temp_uc_timer.data = &ucontrol0;
check_uctemp_timer_cb(&ucontrol0);
@@ -169,6 +258,7 @@ static void signal_handler(int signal)
case SIGINT:
sysmobts_check_temp(no_eeprom_write);
sysmobts_update_hours(no_eeprom_write);
+ close(fd_unix);
exit(0);
break;
case SIGABRT:
diff --git a/src/osmo-bts-sysmo/misc/sysmobts_misc.c
b/src/osmo-bts-sysmo/misc/sysmobts_misc.c
index 9ea26c2..20f7ea7 100644
--- a/src/osmo-bts-sysmo/misc/sysmobts_misc.c
+++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.c
@@ -29,13 +29,17 @@
#include <sys/signal.h>
#include <sys/types.h>
#include <sys/stat.h>
+#include <arpa/inet.h>
#include <osmocom/core/talloc.h>
#include <osmocom/core/utils.h>
#include <osmocom/core/msgb.h>
+#include <osmocom/core/socket.h>
#include <osmocom/core/application.h>
#include <osmocom/vty/telnet_interface.h>
#include <osmocom/vty/logging.h>
+#include <osmocom/gsm/abis_nm.h>
+#include <osmocom/gsm/protocol/ipaccess.h>
#include "btsconfig.h"
#include "sysmobts_misc.h"
@@ -49,10 +53,146 @@
#define SERIAL_ALLOC_SIZE 300
#define SIZE_HEADER_RSP 5
#define SIZE_HEADER_CMD 4
-
+#define OM_ALLOC_SIZE 1024
+#define OM_HEADROOM_SIZE 128
+#define IPA_OML_PROTO 0xFF
#ifdef BUILD_SBTS2050
/**********************************************************************
+ * Function send information to OsmoBts
+ *********************************************************************/
+static void add_sw_descr(struct msgb *msg)
+{
+ char file_version[255];
+ char file_id[255];
+
+ strncpy(file_id, "sysmomgr", sizeof("sysmomgr"));
+ file_id[sizeof(file_id)-1] = '\0';
Comestic nitpick, I prefer:
file_id[sizeof(file_id) - 1] = '\0';
It's debatable anyway.
+ strncpy(file_version, PACKAGE_VERSION,
sizeof(PACKAGE_VERSION));
+ file_version[sizeof(file_version)-1] = '\0';
+ msgb_v_put(msg, NM_ATT_SW_DESCR);
+ msgb_tl16v_put(msg, NM_ATT_FILE_ID, strlen(file_id),
+ (uint8_t *)file_id);
+ msgb_tl16v_put(msg, NM_ATT_FILE_VERSION, strlen(file_version),
+ (uint8_t *)file_version);
+}
+
+static void add_probable_cause(struct msgb *msg)
+{
+ msgb_tv_put(msg, NM_ATT_PROB_CAUSE, NM_PCAUSE_T_MANUF);
+ msgb_v_put(msg, 0);
+ msgb_v_put(msg, 0);
+}
+
+static void add_ipa_header(struct msgb *msg)
+{
+ struct ipaccess_head *hh;
+
+ hh = (struct ipaccess_head *) msgb_push(msg, sizeof(*hh));
+ hh->proto = IPA_OML_PROTO;
+ hh->len = htons(msg->len);
+}
+
+static void add_oml_hdr_msg(struct msgb *msg, uint8_t msg_type,
+ uint8_t obj_class, uint8_t bts_nr,
+ uint8_t trx_nr, uint8_t ts_nr)
+{
+ struct abis_om_fom_hdr *foh;
+ struct abis_om_hdr *omh;
+
+ msg->l3h = msgb_push(msg, sizeof(*foh));
+ foh = (struct abis_om_fom_hdr *) msg->l3h;
+
+ foh->msg_type = msg_type;
+ foh->obj_class = obj_class;
+ foh->obj_inst.bts_nr = bts_nr;
+ foh->obj_inst.trx_nr = trx_nr;
+ foh->obj_inst.ts_nr = ts_nr;
+
+ msg->l2h = msgb_push(msg, sizeof(*omh));
+ omh = (struct abis_om_hdr *) msg->l2h;
+
+ omh->mdisc = ABIS_OM_MDISC_FOM;
+ omh->placement = ABIS_OM_PLACEMENT_ONLY;
+ omh->sequence = 0;
+ omh->length = msgb_l3len(msg);
+}
+
+int sendto_osmobts(int fd_unix, struct uc *ucontrol,
+ enum sbts2050_alert_lvl alert,
+ enum sbts2050_temp_sensor sensor,
+ struct sbts2050_config_info *add_info, int trx_nr)
+{
+ int rc;
+ struct msgb *msg;
+ const char *buf, *nsensor;
+
+ msg = msgb_alloc_headroom(OM_ALLOC_SIZE, OM_HEADROOM_SIZE, "OML");
+ if (msg == NULL) {
+ LOGP(DTEMP, LOGL_ERROR, "Error creating oml msg\n");
+ goto err;
+ }
+
+ add_oml_hdr_msg(msg, NM_MT_FAILURE_EVENT_REP, 0, 0, trx_nr, 0);
+
+ msgb_tv_put(msg, NM_ATT_EVENT_TYPE, NM_EVT_ENV_FAIL);
+
+ switch (alert) {
+ case SBTS2050_WARN_ALERT:
+ msgb_tv_put(msg, NM_ATT_SEVERITY, NM_SEVER_WARNING);
+ break;
+ case SBTS2050_SEVER_ALERT:
+ msgb_tv_put(msg, NM_ATT_SEVERITY, NM_SEVER_CRITICAL);
+ break;
+ default:
+ goto err;
+ }
+
+ add_probable_cause(msg);
+
+ add_sw_descr(msg);
+
+ switch (sensor) {
+ case SBTS2050_TEMP_BOARD:
+ buf = "Unusual temperature on the Board";
+ nsensor = "Board";
+ break;
+ case SBTS2050_TEMP_PA:
+ buf = "Unusual temperature on the PA";
+ nsensor = "PA";
+ break;
+ default:
+ return -1;
+ }
+ memset(add_info->name_sensor, 0, sizeof(add_info->name_sensor));
You don't need to memset this to zero if you ...
+ strncpy(add_info->name_sensor, nsensor,
strlen(nsensor));
^............^
use sizeof up there and nul-terminate this.
add_info->name_sensor[sizeof(add_info->name_sensor) - 1] = '\0';
+
+ msgb_tl16v_put(msg, NM_ATT_ADD_TEXT, strlen(buf), (const uint8_t *)buf);
+
+ /*If we need to send this structure to other machine, we need to pass
+ the integer inside the structure to internet format (big endian)*/
Two comment styles:
/* Blah blah blah */
or, for multi-lines comments:
/* Blah ...
* Blah ...
*/
That is preferred.
+ msgb_tl16v_put(msg, NM_ATT_ADD_INFO,
+ sizeof(struct sbts2050_config_info),
+ (const uint8_t *)add_info);
+
+ add_ipa_header(msg);
+
+ rc = send(fd_unix, msg->data, msg->len, 0);
+ if (rc < 0) {
+ LOGP(DTEMP, LOGL_ERROR, "Error writting in unix socket\n");
+ close(fd_unix);
+ msgb_free(msg);
+ return 0;
+ }
+
+ msgb_free(msg);
+ return 1;
+err:
+ msgb_free(msg);
+ return -1;
+}
+
+/**********************************************************************
* Functions read/write from serial interface
*********************************************************************/
static int hand_serial_read(int fd, struct msgb *msg, int numbytes)
diff --git a/src/osmo-bts-sysmo/misc/sysmobts_misc.h
b/src/osmo-bts-sysmo/misc/sysmobts_misc.h
index 01878f2..2654348 100644
--- a/src/osmo-bts-sysmo/misc/sysmobts_misc.h
+++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.h
@@ -32,6 +32,34 @@ struct ucinfo {
int pa;
};
+enum sbts2050_alert_lvl {
+ SBTS2050_WARN_ALERT,
+ SBTS2050_SEVER_ALERT
+};
+
+enum sbts2050_temp_sensor {
+ SBTS2050_TEMP_BOARD,
+ SBTS2050_TEMP_PA
+};
+
+struct sbts2050_config_info {
+ char name_sensor[8];
+ int temp_max_pa_warn_limit;
+ int temp_min_pa_warn_limit;
+ int temp_max_pa_sever_limit;
+ int temp_min_pa_sever_limit;
+ int temp_max_board_warn_limit;
+ int temp_min_board_warn_limit;
+ int temp_max_board_sever_limit;
+ int temp_min_board_sever_limit;
+ int max_power_red;
+ int slave_power_act;
+ int master_power_act;
+ int pa_power_act;
+ int temp_pa_cur;
+ int temp_board_cur;
+};
+
int sysmobts_temp_get(enum sysmobts_temp_sensor sensor,
enum sysmobts_temp_type type);
@@ -43,6 +71,11 @@ void sbts2050_uc_power(struct uc *ucontrol, int pmaster, int pslave,
int ppa);
int sbts2050_uc_status(struct uc *ucontrol, enum sbts2050_status_rqt status);
+int sendto_osmobts(int fd_unix, struct uc *ucontrol,
+ enum sbts2050_alert_lvl alert,
+ enum sbts2050_temp_sensor sensor,
+ struct sbts2050_config_info *add_info, int trx_nr);
+
int sysmobts_update_hours(int no_epprom_write);
enum sysmobts_firmware_type {
--
1.7.10.4