Re: [osmo-bts PATCH v4] src: Add OML support for sending failure message from manager

Hi Alvaro,

A couple of comments below.

On Wed, Apr 02, 2014 at 01:36:57PM +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

v4: Fixed situation that we have a connection opened with the manager and we have other connection with other manager.

src/osmo-bts-sysmo/main.c | 73 ++++++++++++++++ src/osmo-bts-sysmo/misc/sysmobts_mgr.c | 77 +++++++++++++++++ src/osmo-bts-sysmo/misc/sysmobts_misc.c | 140 ++++++++++++++++++++++++++++++- src/osmo-bts-sysmo/misc/sysmobts_misc.h | 33 ++++++++ 4 files changed, 322 insertions(+), 1 deletion(-)

diff --git a/src/osmo-bts-sysmo/main.c b/src/osmo-bts-sysmo/main.c index 74ee47f..269f442 100644 --- a/src/osmo-bts-sysmo/main.c +++ b/src/osmo-bts-sysmo/main.c @@ -35,6 +35,7 @@

#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>

@@ -45,8 +46,10 @@ #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"

#include "utils.h" #include "eeprom.h" @@ -258,6 +261,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:

@@ -288,6 +292,62 @@ static int write_pid_file(char *procname) 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;
  

• return -1;
  

• }
• msgb_put(msg, rc);
• /*Remove the IPA header for removing the conflict with the new IPA

• * header*/
  

• msgb_pull(msg, sizeof(struct ipaccess_head *)-1);
• mo = &bts->mo;
• msg->trx = mo->bts->c0;
• msg->l2h = msg->data;
• msg->l3h = msg->data + sizeof(struct abis_om_fom_hdr);
• 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 *ofd = (struct osmo_fd *)fd->data;
• if (ofd->fd > -1) {

• close(ofd->fd);
  

• osmo_fd_unregister(ofd);
  

• ofd->fd = -1;
  

• }
• cl = accept(sfd, NULL, NULL);
• if (cl < 0)

• return -1;
  

• ofd->fd = cl;
• if (osmo_fd_register(ofd) != 0) {

• close(cl);
  

• return -1;
  

• }
• return 0;

+}

int main(int argc, char **argv) { struct stat st; @@ -295,6 +355,7 @@ int main(int argc, char **argv) struct gsm_bts_role_bts *btsb; struct e1inp_line *line; void *tall_msgb_ctx;

• struct osmo_fd fd, rfd;

Perhaps you can rename fd and rfd to something a bit more descriptive, eg. accept_ofd and ofd.

int rc;

tall_bts_ctx = talloc_named_const(NULL, 1, "OsmoBTS context"); @@ -370,6 +431,18 @@ int main(int argc, char **argv) fprintf(stderr, "unable to connect to BSC\n"); exit(1); }

• fd.cb = accept_unix_sock;

• rfd.cb = read_sock;

• rfd.when = BSC_FD_READ;

• rfd.fd = -1;

• fd.data = &rfd;

• rc = osmo_sock_unix_init_ofd(&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);
  

• }

  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..525e526 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>

@@ -47,7 +48,11 @@

static int no_eeprom_write = 0; static int daemonize = 0; +static int trx_nr = -1; +static int fd_unix = -1; +static int state_connection = 0;

Please, use some enum for this small state machine, eg.

enum { SYSMO_MGR_DISCONNECTED = 0, SYSMO_MGR_CONNECTED, };

It should help to make the code a bit more readable.

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 +60,29 @@ 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)

^ ^ You can remove those parenthesis.

+/* unix domain socket file descriptor */ +#define SOCKET_PATH "/var/run/bts_oml"

#ifdef BUILD_SBTS2050 +static struct osmo_timer_list connect_timer; +static void socket_connect_cb(void *data) +{

• if (state_connection == 0) {

• fd_unix = osmo_sock_unix_init(SOCK_SEQPACKET, 0, SOCKET_PATH,
  

• 			      OSMO_SOCK_F_CONNECT);
  

• if (fd_unix < 0) {
  

• 	LOGP(DTEMP, LOGL_ERROR, "Error creating unix socket\n");
  

• 	return;
  

• }
  

• state_connection = 1;
  

• }
• osmo_timer_schedule(&connect_timer, CONNECT_TIMER_SECS, 0);

This timer keeps running even if we're connected. I think you should enable it only if we are in disconnected state.

+}

static struct osmo_timer_list temp_uc_timer; static void check_uctemp_timer_cb(void *data) { @@ -64,6 +91,50 @@ 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;
• if (confinfo.temp_min_pa_warn_limit > temp_pa ||

•    confinfo.temp_max_pa_warn_limit < temp_pa) {
  

Should this be >= and <= ?

• state_connection  = sendto_osmobts(fd_unix, ucontrol0,
  

• 				   SBTS2050_WARN_ALERT,
  

• 				   SBTS2050_TEMP_PA,
  

• 				   &confinfo, trx_nr);
  

• } else if (confinfo.temp_min_pa_sever_limit > temp_pa ||

•    confinfo.temp_max_pa_sever_limit < temp_pa) {
  

Same here.

• state_connection = sendto_osmobts(fd_unix, ucontrol0,
  

• 				  SBTS2050_SEVER_ALERT,
  

• 				  SBTS2050_TEMP_PA,
  

• 				  &confinfo, trx_nr);
  

• sbts2050_uc_power(ucontrol0,
  

• 		  sbts2050_uc_status(ucontrol0,
  

• 				     SBTS2050_STATUS_MASTER),
  

• 		  sbts2050_uc_status(ucontrol0,
  

• 				     SBTS2050_STATUS_SLAVE),
  

• 				     confinfo.pa_power_act);
  

• }
• if (confinfo.temp_min_board_warn_limit > temp_board ||

•    confinfo.temp_max_board_warn_limit < temp_board) {
  

• state_connection = sendto_osmobts(fd_unix, ucontrol0,
  

• 				  SBTS2050_WARN_ALERT,
  

• 				  SBTS2050_TEMP_BOARD,
  

• 				  &confinfo, trx_nr);
  

This code looks very similar, perhaps you can encapsulate it in a function, ie.

check_temperature(confinfo.temp_min_board_warn_limit, confinfo.temp_max_board_warn_limit, temp_board, SBTS2050_TEMP_BOARD);

• } else if (confinfo.temp_min_board_sever_limit > temp_board ||

•  confinfo.temp_max_board_sever_limit  < temp_board) {
  

• state_connection = sendto_osmobts(fd_unix, ucontrol0,
  

• 				  SBTS2050_SEVER_ALERT,
  

• 				  SBTS2050_TEMP_BOARD,
  

• 				  &confinfo, trx_nr);
  

• sbts2050_uc_power(ucontrol0, confinfo.master_power_act,
  

• 		  confinfo.slave_power_act,
  

• 		  sbts2050_uc_status(ucontrol0,
  

• 				     SBTS2050_STATUS_PA));
  

• }
• if (state_connection == 0)

• close(fd_unix);
  

• osmo_timer_schedule(&temp_uc_timer, TEMP_TIMER_SECS, 0);

} #endif @@ -93,6 +164,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) {

@@ -169,6 +241,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:

@@ -363,6 +436,10 @@ int main(int argc, char **argv) hours_timer.cb = hours_timer_cb; hours_timer_cb(NULL);

• /*start handle for reconnect the socket in case of error*/

^ ^

Missing spaces.

/* Start handle ... */

• connect_timer.cb = socket_connect_cb;
• socket_connect_cb(NULL);
• /* start uc temperature check timer */ initialize_sbts2050();

diff --git a/src/osmo-bts-sysmo/misc/sysmobts_misc.c b/src/osmo-bts-sysmo/misc/sysmobts_misc.c index 9ea26c2..0e89da6 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,144 @@ #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];
• strcpy(file_id, "sysmomgr");

Better use strncpy here.

• strncpy(file_version, PACKAGE_VERSION, strlen(PACKAGE_VERSION));

And make sure you nul-terminate these strings.

file_version[strlen(PACKAGE_VERSION)-1] = '\0';

As strncpy doesn't append the \0.

I remember you couldn't use strlcpy, right?

• msgb_v_put(msg, NM_ATT_SW_DESCR);
• msgb_tl16v_put(msg, NM_ATT_FILE_ID, strlen(file_id),

• 					    (uint8_t *)file_id);
  

Coding style nitpick, perhaps:

msgb_tl16v_put(msg, NM_ATT_FILE_ID, strlen(file_id), (uint8_t *)file_id);