Hi Jakub,

On Mon, 2019-07-29 at 15:40 +0200, Michał Lowas-Rzechonek wrote:
> From: Jakub Witowski <jakub.witowski@silvair.com>
> 
> This is a small improvement of read_node function readability.
> ---
>  mesh/mesh-config-json.c | 68 +++++++++++++++++++++++++++++--------
> ----
>  1 file changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/mesh/mesh-config-json.c b/mesh/mesh-config-json.c
> index e3baf5dc6..c63883a3d 100644
> --- a/mesh/mesh-config-json.c
> +++ b/mesh/mesh-config-json.c
> @@ -297,6 +297,51 @@ static json_object *jarray_key_del(json_object
> *jarray, int16_t idx)
>  	return jarray_new;
>  }
>  
> +static bool read_unicast_address(json_object *jobj,
> +							uint16_t
> *unicast)
> +{
> +	json_object *jvalue;
> +	char *str;
> +
> +	if (!json_object_object_get_ex(jobj, "unicastAddress",
> &jvalue))
> +		return false;
> +
> +	str = (char *)json_object_get_string(jvalue);
> +	if (sscanf(str, "%04hx", unicast) != 1)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool read_default_ttl(json_object *jobj,
> +								uint8_t
> *ttl)
> +{
> +	json_object *jvalue;
> +	int val;
> +
> +	if (!json_object_object_get_ex(jobj, "defaultTTL", &jvalue))
> +		return false;
> +
> +	val = json_object_get_int(jvalue);
> +
> +	if (val < 0 || val == 1 || val > DEFAULT_TTL)
> +		return false;

New line here to separate from "if" statement

> +	*ttl = (uint8_t) val;
> +
> +	return true;
> +}
> +
> +static bool read_seq_number(json_object *jobj, uint32_t *seq_number)
> +{
> +	json_object *jvalue;
> +
> +	if (!json_object_object_get_ex(jobj, "sequenceNumber",
> &jvalue))
> +		return false;
> +
> +	*seq_number = json_object_get_int(jvalue);
> +	return true;
> +}
> +
>  static bool read_iv_index(json_object *jobj, uint32_t *idx, bool
> *update)
>  {
>  	int tmp;
> @@ -424,7 +469,7 @@ fail:
>  	return false;
>  }
>  
> -static bool read_net_keys(json_object *jobj,  struct
> mesh_config_node *node)
> +static bool read_net_keys(json_object *jobj, struct mesh_config_node
> *node)
>  {
>  	json_object *jarray;
>  	int len;
> @@ -1294,7 +1339,6 @@ static bool read_net_transmit(json_object
> *jobj, struct mesh_config_node *node)
>  static bool read_node(json_object *jnode, struct mesh_config_node
> *node)
>  {
>  	json_object *jvalue;
> -	char *str;
>  
>  	if (!read_iv_index(jnode, &node->iv_index, &node->iv_update)) {
>  		l_info("Failed to read IV index");
> @@ -1318,25 +1362,11 @@ static bool read_node(json_object *jnode,
> struct mesh_config_node *node)
>  
>  	parse_features(jnode, node);
>  
> -	if (!json_object_object_get_ex(jnode, "unicastAddress",
> &jvalue)) {
> -		l_info("Bad config: Unicast address must be present");
> -		return false;
> -	}
> -
> -	str = (char *)json_object_get_string(jvalue);
> -	if (sscanf(str, "%04hx", &node->unicast) != 1)
> -		return false;
> -
> -	if (json_object_object_get_ex(jnode, "defaultTTL", &jvalue)) {
> -		int ttl = json_object_get_int(jvalue);
> +	read_unicast_address(jnode, &node->unicast);
>  

The return status needs to be checked: unicast must be present


> -		if (ttl < 0 || ttl == 1 || ttl > DEFAULT_TTL)
> -			return false;
> -		node->ttl = (uint8_t) ttl;
> -	}
> +	read_default_ttl(jnode, &node->ttl);
>  
I would prefer to keep ttl and sequesnce number parsing as they are
now:
they are not mandatory, but we don't want to allow for bad format,
i.e., we dont want to fail if the fields are absent, but we do want to
fail if the format is wrong.

> -	if (json_object_object_get_ex(jnode, "sequenceNumber",
> &jvalue))
> -		node->seq_number = json_object_get_int(jvalue);
> +	read_seq_number(jnode, &node->seq_number);
>  
>  	/* Check for required "elements" property */
>  	if (!json_object_object_get_ex(jnode, "elements", &jvalue))

Regards,
Inga