Re: [osmo-bts PATCH 1/3 v5] src: Add OML support for sending failure message from manager

Hi Alvaro,

Several comments below.

On Tue, Apr 08, 2014 at 02:31:33PM +0200, Alvaro Neira Ayuso wrote:

From: Álvaro Neira Ayuso anayuso@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@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