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 <[email protected]> > > 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 <[email protected]> > --- > 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);
