Re: [PATCH BlueZ v2 1/3] mesh: Split APP_IDX_DEV into APP_IDX_DEV_LOCAL and APP_IDX_DEV_REMOTE

["Gix, Brian" <brian.gix@intel.com>]

Hi Michał,

This patch fails check patch, plus other issues:

WARNING:LONG_LINE: line over 80 characters
#79: FILE: mesh/model.c:310:
+	if (fwd->idx != APP_IDX_DEV_LOCAL && !has_binding(mod->bindings, fwd->idx))

WARNING:LONG_LINE: line over 80 characters
#119: FILE: mesh/model.c:1386:
+		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV_LOCAL));

WARNING:LONG_LINE: line over 80 characters
#125: FILE: mesh/model.c:1391:
+		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV_LOCAL));

total: 0 errors, 3 warnings, 125 lines checked


On Tue, 2019-07-02 at 11:07 +0200, Michał Lowas-Rzechonek wrote:
> This is needed to distinguish incoming messages encrypted using a device
> key: if the key is local, the message can be forwarded to internal
> models. If the key is a known remote one, it will be forwarded to the
> application via DevKeyMessageReceived() API.
> ---
>  mesh/cfgmod-server.c | 15 ++++++++-------
>  mesh/model.c         | 20 +++++++++++---------
>  mesh/net.h           | 10 ++++++----
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/mesh/cfgmod-server.c b/mesh/cfgmod-server.c
> index 634ac006b..a849b5e99 100644
> --- a/mesh/cfgmod-server.c
> +++ b/mesh/cfgmod-server.c
> @@ -69,7 +69,8 @@ static void send_pub_status(struct mesh_node *node, uint16_t src, uint16_t dst,
>  		n += 2;
>  	}
>  
> -	mesh_model_send(node, dst, src, APP_IDX_DEV, DEFAULT_TTL, msg, n);
> +	mesh_model_send(node, dst, src, APP_IDX_DEV_LOCAL, DEFAULT_TTL,
> +								msg, n);
>  }
>  
>  static bool config_pub_get(struct mesh_node *node, uint16_t src, uint16_t dst,
> @@ -254,7 +255,7 @@ static void send_sub_status(struct mesh_node *node, uint16_t src, uint16_t dst,
>  		n += 2;
>  	}
>  
> -	mesh_model_send(node, dst, src, APP_IDX_DEV, DEFAULT_TTL, msg, n);
> +	mesh_model_send(node, dst, src, APP_IDX_DEV_LOCAL, DEFAULT_TTL, msg, n);
>  }
>  
>  static bool config_sub_get(struct mesh_node *node, uint16_t src, uint16_t dst,
> @@ -312,7 +313,7 @@ static bool config_sub_get(struct mesh_node *node, uint16_t src, uint16_t dst,
>  
>  	*msg_status = (uint8_t) status;
>  
> -	mesh_model_send(node, dst, src, APP_IDX_DEV, DEFAULT_TTL, msg, n);
> +	mesh_model_send(node, dst, src, APP_IDX_DEV_LOCAL, DEFAULT_TTL, msg, n);
>  	return true;
>  }
>  
> @@ -487,7 +488,7 @@ static void send_model_app_status(struct mesh_node *node, uint16_t src,
>  	l_put_le16(id, msg + n);
>  	n += 2;
>  
> -	mesh_model_send(node, dst, src, APP_IDX_DEV, DEFAULT_TTL, msg, n);
> +	mesh_model_send(node, dst, src, APP_IDX_DEV_LOCAL, DEFAULT_TTL, msg, n);
>  }
>  
>  static void model_app_list(struct mesh_node *node, uint16_t src, uint16_t dst,
> @@ -540,7 +541,7 @@ static void model_app_list(struct mesh_node *node, uint16_t src, uint16_t dst,
>  
>  	if (result >= 0) {
>  		*status = result;
> -		mesh_model_send(node, dst, src, APP_IDX_DEV, DEFAULT_TTL,
> +		mesh_model_send(node, dst, src, APP_IDX_DEV_LOCAL, DEFAULT_TTL,
>  								msg, n);
>  	}
>  
> @@ -758,7 +759,7 @@ static bool cfg_srv_pkt(uint16_t src, uint32_t dst,
>  	uint16_t interval;
>  	uint16_t n;
>  
> -	if (idx != APP_IDX_DEV)
> +	if (idx != APP_IDX_DEV_LOCAL)
>  		return false;
>  
>  	if (mesh_model_opcode_get(pkt, size, &opcode, &n)) {
> @@ -1259,7 +1260,7 @@ static bool cfg_srv_pkt(uint16_t src, uint32_t dst,
>  	if (n) {
>  		/* print_packet("App Tx", long_msg ? long_msg : msg, n); */
>  		mesh_model_send(node, unicast, src,
> -				APP_IDX_DEV, DEFAULT_TTL,
> +				APP_IDX_DEV_LOCAL, DEFAULT_TTL,
>  				long_msg ? long_msg : msg, n);
>  	}
>  	l_free(long_msg);
> diff --git a/mesh/model.c b/mesh/model.c
> index 7401dcecb..e09dbd364 100644
> --- a/mesh/model.c
> +++ b/mesh/model.c
> @@ -306,7 +306,8 @@ static void forward_model(void *a, void *b)
>  	bool result;
>  
>  	l_debug("model %8.8x with idx %3.3x", mod->id, fwd->idx);
> -	if (fwd->idx != APP_IDX_DEV && !has_binding(mod->bindings, fwd->idx))
> +
> +	if (fwd->idx != APP_IDX_DEV_LOCAL && !has_binding(mod->bindings, fwd->idx))
>  		return;
>  
>  	dst = fwd->dst;
> @@ -356,15 +357,16 @@ static int dev_packet_decrypt(struct mesh_node *node, const uint8_t *data,
>  				uint16_t dst, uint8_t key_id, uint32_t seq,
>  				uint32_t iv_idx, uint8_t *out)
>  {
> -	const uint8_t *dev_key;
> +	uint8_t dev_key[16];
> +	const uint8_t *key;

You have renamed dev_key --> key, and no longer use dev_key that I can see here...  which will cause it's own
build warning:

mesh/model.c: In function ‘dev_packet_decrypt’:
mesh/model.c:360:10: error: unused variable ‘dev_key’ [-Werror=unused-variable]
  uint8_t dev_key[16];
          ^~~~~~~
cc1: all warnings being treated as errors


>  
> -	dev_key = node_get_device_key(node);
> -	if (!dev_key)
> +	key = node_get_device_key(node);
> +	if (!key)
>  		return false;
>  
>  	if (mesh_crypto_payload_decrypt(NULL, 0, data, size, szmict, src,
> -					dst, key_id, seq, iv_idx, out, dev_key))
> -		return APP_IDX_DEV;
> +					dst, key_id, seq, iv_idx, out, key))
> +		return APP_IDX_DEV_LOCAL;
>  
>  	return -1;
>  }
> @@ -952,7 +954,7 @@ bool mesh_model_send(struct mesh_node *node, uint16_t src, uint16_t target,
>  	if (IS_UNASSIGNED(target))
>  		return false;
>  
> -	if (app_idx == APP_IDX_DEV) {
> +	if (app_idx == APP_IDX_DEV_LOCAL) {
>  		key = node_get_device_key(node);
>  		if (!key)
>  			return false;
> @@ -1381,12 +1383,12 @@ struct mesh_model *mesh_model_setup(struct mesh_node *node, uint8_t ele_idx,
>  		if (ele_idx != PRIMARY_ELE_IDX)
>  			return NULL;
>  
> -		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV));
> +		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV_LOCAL));
>  		return mod;
>  	}
>  
>  	if (db_mod->id == CONFIG_CLI_MODEL) {
> -		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV));
> +		l_queue_push_head(mod->bindings, L_UINT_TO_PTR(APP_IDX_DEV_LOCAL));
>  		return mod;
>  	}
>  
> diff --git a/mesh/net.h b/mesh/net.h
> index f19024766..834712d8d 100644
> --- a/mesh/net.h
> +++ b/mesh/net.h
> @@ -37,10 +37,12 @@ struct mesh_node;
>  #define SEQ_MASK	0xffffff
>  
>  #define CREDFLAG_MASK	0x1000
> -#define APP_IDX_MASK	0x0fff
> -#define APP_IDX_DEV	0x7fff
> -#define APP_IDX_ANY	0x8000
> -#define APP_IDX_NET	0xffff
> +
> +#define APP_IDX_MASK		0x0fff
> +#define APP_IDX_DEV_REMOTE	0x6fff
> +#define APP_IDX_DEV_LOCAL	0x7fff
> +#define APP_IDX_ANY		0x8000
> +#define APP_IDX_NET		0xffff

Since this patch-set needs to be re-spun, Inga was hoping you could also remove the unused cruft APP_IDX_NET
altogether...


>  
>  #define NET_IDX_INVALID	0xffff
>  #define NET_NID_INVALID	0xff