[PATCH 0/3] Allow to reattach with new composition data

[PATCH 0/3] Allow to reattach with new composition data

[Jakub Witowski]

This patch allows the application to modify the CID, PID, VID and CRPL in the composition data.

According the Mesh Profile (2.3.4 Elements) the modification of fields other than "Elements" is not prohibited.

Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to the node_composition struct.
The static is more clear and less problematic.

Jakub Witowski (3):
  mesh: use static node_comp instead of the pointer
  mesh: add composition data setter
  mesh: allow to reattach with new composition data

 mesh/mesh-config-json.c | 46 +++++++++++++++-----
 mesh/mesh-config.h      |  2 +
 mesh/node.c             | 96 +++++++++++++++++++++++++----------------
 3 files changed, 96 insertions(+), 48 deletions(-)

-- 
2.20.1

[PATCH 1/3] mesh: use static node_comp instead of the pointer

[Jakub Witowski]

---
 mesh/node.c | 56 +++++++++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index de6e74c4f..6fe70742d 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -90,7 +90,7 @@ struct mesh_node {
 	uint32_t seq_number;
 	bool provisioner;
 	uint16_t primary;
-	struct node_composition *comp;
+	struct node_composition comp;
 	struct {
 		uint16_t interval;
 		uint8_t cnt;
@@ -336,7 +336,6 @@ static void free_node_resources(void *data)
 	free_node_dbus_resources(node);
 
 	mesh_net_free(node->net);
-	l_free(node->comp);
 	l_free(node->storage_dir);
 	l_free(node);
 }
@@ -503,11 +502,10 @@ static bool init_from_storage(struct mesh_config_node *db_node,
 
 	l_queue_push_tail(nodes, node);
 
-	node->comp = l_new(struct node_composition, 1);
-	node->comp->cid = db_node->cid;
-	node->comp->pid = db_node->pid;
-	node->comp->vid = db_node->vid;
-	node->comp->crpl = db_node->crpl;
+	node->comp.cid = db_node->cid;
+	node->comp.pid = db_node->pid;
+	node->comp.vid = db_node->vid;
+	node->comp.crpl = db_node->crpl;
 	node->lpn = db_node->modes.lpn;
 
 	node->proxy = db_node->modes.proxy;
@@ -753,7 +751,7 @@ uint16_t node_get_crpl(struct mesh_node *node)
 	if (!node)
 		return 0;
 
-	return node->comp->crpl;
+	return node->comp.crpl;
 }
 
 uint8_t node_relay_mode_get(struct mesh_node *node, uint8_t *count,
@@ -886,18 +884,18 @@ uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 	uint16_t num_ele = 0;
 	const struct l_queue_entry *ele_entry;
 
-	if (!node || !node->comp || sz < MIN_COMP_SIZE)
+	if (!node || sz < MIN_COMP_SIZE)
 		return 0;
 
 	n = 0;
 
-	l_put_le16(node->comp->cid, buf + n);
+	l_put_le16(node->comp.cid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->pid, buf + n);
+	l_put_le16(node->comp.pid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->vid, buf + n);
+	l_put_le16(node->comp.vid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->crpl, buf + n);
+	l_put_le16(node->comp.crpl, buf + n);
 	n += 2;
 
 	features = 0;
@@ -1198,10 +1196,10 @@ static void convert_node_to_storage(struct mesh_node *node,
 {
 	const struct l_queue_entry *entry;
 
-	db_node->cid = node->comp->cid;
-	db_node->pid = node->comp->pid;
-	db_node->vid = node->comp->vid;
-	db_node->crpl = node->comp->crpl;
+	db_node->cid = node->comp.cid;
+	db_node->pid = node->comp.pid;
+	db_node->vid = node->comp.vid;
+	db_node->crpl = node->comp.crpl;
 	db_node->modes.lpn = node->lpn;
 	db_node->modes.proxy = node->proxy;
 
@@ -1285,29 +1283,28 @@ static bool get_app_properties(struct mesh_node *node, const char *path,
 
 	l_debug("path %s", path);
 
-	node->comp = l_new(struct node_composition, 1);
-	node->comp->crpl = mesh_get_crpl();
+	node->comp.crpl = mesh_get_crpl();
 
 	while (l_dbus_message_iter_next_entry(properties, &key, &variant)) {
 		if (!cid && !strcmp(key, "CompanyID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->cid))
-				goto fail;
+							&node->comp.cid))
+				return false;
 			cid = true;
 			continue;
 		}
 
 		if (!pid && !strcmp(key, "ProductID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->pid))
-				goto fail;
+							&node->comp.pid))
+				return false;
 			pid = true;
 			continue;
 		}
 
 		if (!vid && !strcmp(key, "VersionID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->vid))
+							&node->comp.vid))
 				return false;
 			vid = true;
 			continue;
@@ -1315,21 +1312,16 @@ static bool get_app_properties(struct mesh_node *node, const char *path,
 
 		if (!strcmp(key, "CRPL")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->crpl))
-				goto fail;
+							&node->comp.crpl))
+				return false;
 			continue;
 		}
 	}
 
 	if (!cid || !pid || !vid)
-		goto fail;
+		return false;
 
 	return true;
-fail:
-	l_free(node->comp);
-	node->comp = NULL;
-
-	return false;
 }
 
 static bool add_local_node(struct mesh_node *node, uint16_t unicast, bool kr,
-- 
2.20.1

[PATCH 2/3] mesh: add composition data setter

[Jakub Witowski]

---
 mesh/mesh-config-json.c | 46 +++++++++++++++++++++++++++++++----------
 mesh/mesh-config.h      |  2 ++
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/mesh/mesh-config-json.c b/mesh/mesh-config-json.c
index 5855149e3..ee42cf7df 100644
--- a/mesh/mesh-config-json.c
+++ b/mesh/mesh-config-json.c
@@ -1456,6 +1456,27 @@ static bool write_mode(json_object *jobj, const char *keyword, int value)
 	return true;
 }
 
+static bool write_comp(json_object *jobj, uint16_t cid, uint16_t pid,
+						uint16_t vid, uint16_t crpl)
+{
+	if (!jobj)
+		return false;
+
+	if (!write_uint16_hex(jobj, "cid", cid))
+		return false;
+
+	if (!write_uint16_hex(jobj, "pid", pid))
+		return false;
+
+	if (!write_uint16_hex(jobj, "vid", vid))
+		return false;
+
+	if (!write_uint16_hex(jobj, "crpl", crpl))
+		return false;
+
+	return true;
+}
+
 bool mesh_config_write_mode(struct mesh_config *cfg, const char *keyword,
 								int value)
 {
@@ -1595,17 +1616,8 @@ static struct mesh_config *create_config(const char *cfg_path,
 
 	jnode = json_object_new_object();
 
-	/* CID, PID, VID, crpl */
-	if (!write_uint16_hex(jnode, "cid", node->cid))
-		return NULL;
-
-	if (!write_uint16_hex(jnode, "pid", node->pid))
-		return NULL;
-
-	if (!write_uint16_hex(jnode, "vid", node->vid))
-		return NULL;
-
-	if (!write_uint16_hex(jnode, "crpl", node->crpl))
+	/* CID, PID, VID, CRPL */
+	if (!write_comp(jnode, node->cid, node->pid, node->vid, node->crpl))
 		return NULL;
 
 	/* Features: relay, LPN, friend, proxy*/
@@ -2052,6 +2064,18 @@ bool mesh_config_write_ttl(struct mesh_config *cfg, uint8_t ttl)
 	return save_config(cfg->jnode, cfg->node_dir_path);
 }
 
+bool mesh_config_write_comp(struct mesh_config *cfg, uint16_t cid, uint16_t pid,
+						uint16_t vid, uint16_t crpl)
+{
+	if (!cfg)
+		return false;
+
+	if (!write_comp(cfg->jnode, cid, pid, vid, crpl))
+		return false;
+
+	return true;
+}
+
 static bool load_node(const char *fname, const uint8_t uuid[16],
 				mesh_config_node_func_t cb, void *user_data)
 {
diff --git a/mesh/mesh-config.h b/mesh/mesh-config.h
index a5b12bbad..5a003b95c 100644
--- a/mesh/mesh-config.h
+++ b/mesh/mesh-config.h
@@ -135,6 +135,8 @@ bool mesh_config_write_unicast(struct mesh_config *cfg, uint16_t unicast);
 bool mesh_config_write_relay_mode(struct mesh_config *cfg, uint8_t mode,
 					uint8_t count, uint16_t interval);
 bool mesh_config_write_ttl(struct mesh_config *cfg, uint8_t ttl);
+bool mesh_config_write_comp(struct mesh_config *cfg, uint16_t cid, uint16_t pid,
+						uint16_t vid, uint16_t crpl);
 bool mesh_config_write_mode(struct mesh_config *cfg, const char *keyword,
 								int value);
 bool mesh_config_model_binding_add(struct mesh_config *cfg, uint16_t ele_addr,
-- 
2.20.1

[PATCH 3/3] mesh: allow to reattach with new composition data

[Jakub Witowski]

---
 mesh/node.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index 6fe70742d..f9a2d5722 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -47,6 +47,12 @@
 
 #define MIN_COMP_SIZE 14
 
+/* Composition data header size contains the length of belows:
+ * CID, PID, VID, CRPL and Feature bits
+ */
+#define COMP_HDR_SIZE 10
+#define COMP_FEATURE_BITS_SIZE 2
+
 #define MESH_NODE_PATH_PREFIX "/node"
 
 /* Default values for a new locally created node */
@@ -1394,15 +1400,39 @@ static bool check_req_node(struct managed_obj_request *req)
 		uint16_t attach_len = node_generate_comp(req->attach,
 					attach_comp, sizeof(attach_comp));
 
-		/* Ignore feature bits in Composition Compare */
-		node_comp[8] = 0;
-		attach_comp[8] = 0;
-
+		/* Ignore CID, VID, PID, CRPL and feature bits
+		 * in Composition Compare
+		 */
 		if (node_len != attach_len ||
-				memcmp(node_comp, attach_comp, node_len)) {
+			memcmp(node_comp + COMP_HDR_SIZE,
+						attach_comp + COMP_HDR_SIZE,
+						node_len - COMP_HDR_SIZE)) {
 			l_debug("Failed to verify app's composition data");
 			return false;
 		}
+
+		/* Compare CID, VID, PID and CRPL */
+		if (!memcmp(node_comp, attach_comp,
+					COMP_HDR_SIZE - COMP_FEATURE_BITS_SIZE))
+			return true;
+
+		l_debug("Composition data update");
+
+		if (!mesh_config_write_comp(req->attach->cfg,
+				req->node->comp.cid, req->node->comp.pid,
+				req->node->comp.vid, req->node->comp.crpl)) {
+
+			l_debug("Failed to update composition data");
+			return false;
+		}
+
+		if (!mesh_config_save(req->attach->cfg, true, NULL, NULL)) {
+			l_debug("Failed to store composition data");
+			return false;
+		}
+
+		memcpy(&req->attach->comp, &req->node->comp,
+					sizeof(struct node_composition));
 	}
 
 	return true;
-- 
2.20.1

Re: [PATCH 0/3] Allow to reattach with new composition data

[Gix, Brian]

Hi Jakub,

a few things..

First, the subject line for user space patches should always be [BlueZ PATCH...] to differentiate bluez.git
patches from kernel patches.

Also:  The title of the patch should always start with the component being modified... in this case 
"mesh: Allow reattach..." or "tools/mesh: XXXX" for example

On Mon, 2020-01-20 at 17:11 +0100, Jakub Witowski wrote:
> This patch allows the application to modify the CID, PID, VID and CRPL in the composition data.

This will require some discussion.   Currently we are planning at *least* to make CRPL entirely under the
control of bluetooth-mesh.conf, because this has a significant daemon impact.

The other settings I am not as concerned about...  If the spec really does allow their alteration though, I am
willing to consider them.

> According the Mesh Profile (2.3.4 Elements) the modification of fields other than "Elements" is not
> prohibited.
> 
> Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to the node_composition
> struct.
> The static is more clear and less problematic.
> 
> Jakub Witowski (3):
>   mesh: use static node_comp instead of the pointer
>   mesh: add composition data setter
>   mesh: allow to reattach with new composition data
> 
>  mesh/mesh-config-json.c | 46 +++++++++++++++-----
>  mesh/mesh-config.h      |  2 +
>  mesh/node.c             | 96 +++++++++++++++++++++++++----------------
>  3 files changed, 96 insertions(+), 48 deletions(-)
> 

Re: [PATCH 0/3] Allow to reattach with new composition data

[jakub.witowski]

On 01/20, Gix, Brian wrote:
> Hi Jakub,
> 
> a few things..
> 
> First, the subject line for user space patches should always be [BlueZ PATCH...] to differentiate bluez.git
> patches from kernel patches.
Correct, sorry for that.

> 
> Also:  The title of the patch should always start with the component being modified... in this case 
> "mesh: Allow reattach..." or "tools/mesh: XXXX" for example
> 
Ok

> On Mon, 2020-01-20 at 17:11 +0100, Jakub Witowski wrote:
> > This patch allows the application to modify the CID, PID, VID and CRPL in the composition data.
> 
> This will require some discussion.   Currently we are planning at *least* to make CRPL entirely under the
> control of bluetooth-mesh.conf, because this has a significant daemon impact.

Ok, I don't have a problem with that. I guess this is going to behave in
the same way as feature bits, and CRPL will disappear from Application1
properties altogether?

> 
> The other settings I am not as concerned about...  If the spec really does allow their alteration though, I am
> willing to consider them.
> 

It doesn't explicitly allow the update, but doesn't forbid it
either... These values are application properties, and particularly the
VID can change after software upgrade, (even if element layout doesn't,
that would require re-provisioning etc.). Supporting this is the main
use case.

> > According the Mesh Profile (2.3.4 Elements) the modification of fields other than "Elements" is not
> > prohibited.
> > 
> > Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to the node_composition
> > struct.
> > The static is more clear and less problematic.
> > 
> > Jakub Witowski (3):
> >   mesh: use static node_comp instead of the pointer
> >   mesh: add composition data setter
> >   mesh: allow to reattach with new composition data
> > 
> >  mesh/mesh-config-json.c | 46 +++++++++++++++-----
> >  mesh/mesh-config.h      |  2 +
> >  mesh/node.c             | 96 +++++++++++++++++++++++++----------------
> >  3 files changed, 96 insertions(+), 48 deletions(-)
> > 

Re: [PATCH 0/3] Allow to reattach with new composition data

[Stotland, Inga]

Hi Jakub,

On Tue, 2020-01-21 at 11:59 +0100, jakub.witowski@silvair.com wrote:
> On 01/20, Gix, Brian wrote:
> > Hi Jakub,
> > 
> > a few things..
> > 
> > First, the subject line for user space patches should always be [BlueZ PATCH...] to differentiate bluez.git
> > patches from kernel patches.
> Correct, sorry for that.
> 
> > 
> > Also:  The title of the patch should always start with the component being modified... in this case 
> > "mesh: Allow reattach..." or "tools/mesh: XXXX" for example
> > 
> Ok
> 
> > On Mon, 2020-01-20 at 17:11 +0100, Jakub Witowski wrote:
> > > This patch allows the application to modify the CID, PID, VID and CRPL in the composition data.
> > 
> > This will require some discussion.   Currently we are planning at *least* to make CRPL entirely under the
> > control of bluetooth-mesh.conf, because this has a significant daemon impact.
> 
> Ok, I don't have a problem with that. I guess this is going to behave in
> the same way as feature bits, and CRPL will disappear from Application1
> properties altogether?

I think that the value of CRPL in the config keyfile should correspond
to the maximum CRPL value that the node supports. If an application
specifies anything greater than config value, it will be capped.

As far as changing the value of CRPL dynamically (i.e., in app's
properties), we may allow it, but *only* if the new CRPL value is
greater than the previously specified one (again, subject to be capped
at max).

> 
> > 
> > The other settings I am not as concerned about...  If the spec really does allow their alteration though, I am
> > willing to consider them.
> > 
> 
> It doesn't explicitly allow the update, but doesn't forbid it
> either... These values are application properties, and particularly the
> VID can change after software upgrade, (even if element layout doesn't,
> that would require re-provisioning etc.). Supporting this is the main
> use case.
> 

I would strongly prefer not to allow changing CID. Is there a good
reason to change CID ever?
Changing product ID and especially version ID ahould be fine.

> > > According the Mesh Profile (2.3.4 Elements) the modification of fields other than "Elements" is not
> > > prohibited.
> > > 
> > > Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to the node_composition
> > > struct.
> > > The static is more clear and less problematic.
> > > 

Agree

> > > Jakub Witowski (3):
> > >   mesh: use static node_comp instead of the pointer
> > >   mesh: add composition data setter
> > >   mesh: allow to reattach with new composition data
> > > 
> > >  mesh/mesh-config-json.c | 46 +++++++++++++++-----
> > >  mesh/mesh-config.h      |  2 +
> > >  mesh/node.c             | 96 +++++++++++++++++++++++++----------------
> > >  3 files changed, 96 insertions(+), 48 deletions(-)
> > > 

Best regards,
Inga

Re: [PATCH 0/3] Allow to reattach with new composition data

[Michał Lowas-Rzechonek]

Hi Inga,

On 01/21, Stotland, Inga wrote:
> I would strongly prefer not to allow changing CID. Is there a good
> reason to change CID ever?

I don't know, maybe legal, after acquisition, or something?

I don't see any practical difference between these 3 IDs - nothing in
the spec is tied to them, they are informational only. So maybe keep
things simple and not add any "special" logic around the CID?

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH 0/3] Allow to reattach with new composition data

[jakub.witowski]

Hi Inga,
On 01/21, Michał Lowas-Rzechonek wrote:
> Hi Inga,
> 
> On 01/21, Stotland, Inga wrote:
> > I would strongly prefer not to allow changing CID. Is there a good
> > reason to change CID ever?
> 
> I don't know, maybe legal, after acquisition, or something?
> 
> I don't see any practical difference between these 3 IDs - nothing in
> the spec is tied to them, they are informational only. So maybe keep
> things simple and not add any "special" logic around the CID?
I fully agree with Michal. Do you have a reason to treat the CID
in a different way than PID and VID ? It does not seems to have a
negative impact for the BlueZ.
> 
> -- 
> Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
> Silvair http://silvair.com
> Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH 0/3] Allow to reattach with new composition data

[Michał Lowas-Rzechonek]

On 01/20, Gix, Brian wrote:
> On Mon, 2020-01-20 at 17:11 +0100, Jakub Witowski wrote:
> > This patch allows the application to modify the CID, PID, VID and
> > CRPL in the composition data.
> 
> This will require some discussion.   Currently we are planning at
> *least* to make CRPL entirely under the control of
> bluetooth-mesh.conf, because this has a significant daemon impact.

Alright, but at the moment the daemon accepts whatever the application
specifies. The proposed patch doesn't change that, so maybe let's keep
it that way until a dedicated change in CRPL behaviour lands?

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH 0/3] Allow to reattach with new composition data

[Michał Lowas-Rzechonek]

On 01/22, Michał Lowas-Rzechonek wrote:
> On 01/20, Gix, Brian wrote:
> > This will require some discussion.   Currently we are planning at
> > *least* to make CRPL entirely under the control of
> > bluetooth-mesh.conf, because this has a significant daemon impact.
> 
> Alright, but at the moment the daemon accepts whatever the application
> specifies. The proposed patch doesn't change that, so maybe let's keep
> it that way until a dedicated change in CRPL behaviour lands?

By the way, would you like us to prepare such a change?

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND