Re: [PATCH 2/5] netmon: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]

Hi Djalal,

On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
> From: Djalal Harouni <djalal@endocode.com>
>
> Handle the previously added types in D-Bus.
> ---
>   src/netmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/src/netmon.c b/src/netmon.c
> index eb18b9c..c87980a 100644
> --- a/src/netmon.c
> +++ b/src/netmon.c
> @@ -78,7 +78,8 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
>   	const char *technology = cell_type_to_tech_name(type);
>   	char *mcc = NULL;
>   	char *mnc = NULL;
> -	int intval;
> +	char *op = NULL;
> +	int intval = -1;

Why the initializer?  Our rule of thumb is not to initialize in order to 
let the compiler (hopefully) or valgrind catch instances of 
uninitialized variable use (e.g. bugs).

>   	netmon->reply = dbus_message_new_method_return(netmon->pending);
>
>   	if (netmon->reply == NULL)
> @@ -180,6 +181,43 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
>   					intval, uint8_t, DBUS_TYPE_BYTE);
>   			break;
>
> +		case OFONO_NETMON_INFO_RSCP:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReceivedSignalCodePower",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_ECN0:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReceivedEnergyRatio",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_RSRQ:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedQuality",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_RSRP:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedPower",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +

This looks fine, however there should be a patch documenting these and 
their respective value ranges inside doc/netmon-api.txt.

> +		case OFONO_NETMON_INFO_OPERATOR:
> +			op = va_arg(arglist, char *);
> +
> +			if (op && strlen(op))
> +				ofono_dbus_dict_append(&dict, "Operator",
> +						DBUS_TYPE_STRING, &op);
> +
> +			break;
> +
>   		case OFONO_NETMON_INFO_INVALID:
>   			break;
>   		}
>

Regards,
-Denis