[osmo-bts PATCH 4/4 v8] main: Added support for changing the power transmitter in sbts2050

[Holger Hans Peter Freyther]

On Mon, May 05, 2014 at 04:22:59PM +0200, Alvaro Neira Ayuso wrote:
> From: Álvaro Neira Ayuso <anayuso at sysmocom.de>
> 
> Added functions for changing the power transmitter in case of we receive

				transmit power?

> a Failure Event report from the manager. The sbts2050 decress the

This is not what happens. Is it? The manager will ask for the power being
reduced. It is not related to a OML Failure Event report.

decress.. do you mean decrease?


> power transmitter with the value that we have configured in the manager.

> +	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));
> +	}

Same as in the other mail. Maybe even share this with a function
to put the osmocom manufacturer header.

> +	prepend_oml_ipa_header(msg);
> +
> +	rc = send(fd_unix, msg->data, msg->len, 0);
> +	if (rc < 0 || rc != msg->len) {
> +		LOGP(DTEMP, LOGL_ERROR, "Error writting in unix socket\n");
> +		close(fd_unix);
> +		msgb_free(msg);
> +		return -1;
> +	}
> +
> +	return rc;

Are you leaking the msgb here? I think I saw similar code in the
previous patch. For previous leaks I asked you to send the messages
in a loop to make it obvious that some meoyr is leaked..

> +	if ((pa_warning || board_warning) &&
> +	    status_change_power_red == SBTS2050_DISABLE_CHANGE_POWER) {
> +		status_change_power_red = SBTS2050_ENABLE_CHANGE_POWER;
> +		send_omlreduce(fd_unix, confinfo.reduce_max_power, trx_nr);
> +	} else if (!pa_warning && !board_warning &&
> +		   status_change_power_red == SBTS2050_ENABLE_CHANGE_POWER) {
> +		status_change_power_red = SBTS2050_DISABLE_CHANGE_POWER;
> +		send_omlreduce(fd_unix, 0, trx_nr);
> +	}

Move this into a method. We certainly will change the code in the
future to not just go from 0 to X for power changes. The method
should take pa_warning, board_warning, the state and it should
return -1 if no change is required and otherwise the power it
should be reduced to.


> +	rc = recv(fd_unix, msg->tail, msg->data_len, 0);
> +	if (rc <= 0) {
> +		LOGP(DTEMP, LOGL_ERROR, "Error reading from unix socket\n");

print the kind of error like in the error log messages.