[osmo-bts PATCH 2/4 v12] src: Add OML support for sending failure message from manager

[Holger Hans Peter Freyther]

On Mon, May 05, 2014 at 04:09:51PM +0200, Alvaro Neira Ayuso wrote:
> 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.

Sorry but the above is not too clear. Please try again.

> +static struct osmo_timer_list connect_timer;
> +static void socket_connect_cb(void *data)
> +{
> +	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);

What prevents socket_connect_cb(NULL) be called _while_ the
timer is pending? While timers for check_temperature and
socket_connect_cb might match now one should make sure the
timer is stopped.



> +static int check_temperature(struct uc *ucontrol0, int lowlimit, int highlimit,
> @@ -169,6 +260,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);

Well, if we close it we should set fd_unix to -1 and unregister
the fd. This might not be signal-safe, so why to close it at all?

> +	char file_version[255];
> +	char file_id[255];
> +
> +	strncpy(file_id, "sysmomgr", sizeof("sysmomgr"));
> +	file_id[sizeof(file_id) - 1] = '\0';
> +	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);

That is odd. You use "sizeof" which includes the NUL and then you
use strlen on file_id which will remove the NUL. This appears to
be not consistent.

> +	if (is_manuf) {
> +		/* length byte, string + 0 termination */
> +		uint8_t *manuf = msgb_push(msg, 1 + sizeof(osmocom_magic));
> +		manuf[0] = strlen(osmocom_magic)+1;
> +		memcpy(manuf+1, osmocom_magic, strlen(osmocom_magic));
> +	}

The sizes look odd here. Sure you want to have one byte for the
length, the string and the NUL byte but you expect/assume that the
msg is NUL at strlen(osmocom_magic)+1. Maybe it is better to not make
this assumption?

> +	strncpy(add_info->name_sensor, nsensor, sizeof(add_info->name_sensor));

> +	/* If we need to send this structure to other machine, we need to pass
> +	 * the integer inside the structure to internet format (big endian)
> +	 */
> +	msgb_tl16v_put(msg, NM_ATT_ADD_INFO,
> +		       sizeof(struct sbts2050_config_info),
> +		       (const uint8_t *)add_info);

yes? but it is sent to a BSC which might be another machine? Is this
relevant?