diff mbox series

[BlueZ,2/4] mesh: Update UnprovisionedScan, AddNode & ScanResult

Message ID 20200327184257.15042-3-inga.stotland@intel.com (mailing list archive)
State Accepted
Delegated to: Brian Gix
Headers show
Series API changes for forward compatibility | expand

Commit Message

Stotland, Inga March 27, 2020, 6:42 p.m. UTC 
The following methods are modified to allow for future development:

Interface org.bluez.mesh.Management1:

Old: void UnprovisionedScan(uint16 seconds)
New: void UnprovisionedScan(dict options)

    The options parameter is a dictionary with the following keys defined:
    uint16 Seconds
                Specifies number of seconds for scanning to be active.
                If set to 0 or if this key is not present, then the
                scanning will continue until UnprovisionedScanCancel()
                or AddNode() methods are called.
    other keys TBD

Old: void AddNode(array{byte}[16] uuid)
New: void AddNode(array{byte}[16] uuid, dict options)

    The options parameter is currently an empty dictionary

Interface org.bluez.mesh.Provisioner1

Old: void ScanResult(int16 rssi, array{byte} data)
New: void ScanResult(int16 rssi, array{byte} data, dict options)

    The options parameter is currently an empty dictionary
---
 mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

Comments

Brian Gix March 27, 2020, 9:17 p.m. UTC | #1 
Hi Inga,
On Fri, 2020-03-27 at 11:42 -0700, Inga Stotland wrote:
> The following methods are modified to allow for future development:
> 
> Interface org.bluez.mesh.Management1:
> 
> Old: void UnprovisionedScan(uint16 seconds)
> New: void UnprovisionedScan(dict options)
> 
>     The options parameter is a dictionary with the following keys defined:
>     uint16 Seconds
>                 Specifies number of seconds for scanning to be active.
>                 If set to 0 or if this key is not present, then the
>                 scanning will continue until UnprovisionedScanCancel()
>                 or AddNode() methods are called.
>     other keys TBD
> 
> Old: void AddNode(array{byte}[16] uuid)
> New: void AddNode(array{byte}[16] uuid, dict options)
> 
>     The options parameter is currently an empty dictionary
> 
> Interface org.bluez.mesh.Provisioner1
> 
> Old: void ScanResult(int16 rssi, array{byte} data)
> New: void ScanResult(int16 rssi, array{byte} data, dict options)
> 
>     The options parameter is currently an empty dictionary
> ---
>  mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/mesh/manager.c b/mesh/manager.c
> index 0909c7e16..8e948e47d 100644
> --- a/mesh/manager.c
> +++ b/mesh/manager.c
> @@ -217,21 +217,22 @@ static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
>  						void *user_data)
>  {
>  	struct mesh_node *node = user_data;
> -	struct l_dbus_message_iter iter_uuid;
> +	struct l_dbus_message_iter iter_uuid, options;
>  	struct l_dbus_message *reply;
>  	uint8_t *uuid;
> -	uint32_t n;
> +	uint32_t n = 22;
>  
>  	l_debug("AddNode request");
>  
> -	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
> +	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
>  	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
> -								|| n != 16)
> +	    || n != 16) {
> +		l_debug("n = %u", n);
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
>  							"Bad device UUID");
> -
> +	}
>  	/* Allow AddNode to cancel Scanning if from the same node */
>  	if (scan_node) {
>  		if (scan_node != node)
> @@ -361,6 +362,9 @@ static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
>  	builder = l_dbus_message_builder_new(msg);
>  	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
>  	dbus_append_byte_array(builder, data + 2, len -2);
> +	l_dbus_message_builder_enter_array(builder, "{sv}");
> +	/* TODO: populate with options when defined */
> +	l_dbus_message_builder_leave_array(builder);
>  	l_dbus_message_builder_finalize(builder);
>  	l_dbus_message_builder_destroy(builder);
>  
> @@ -372,17 +376,34 @@ static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
>  						void *user_data)
>  {
>  	struct mesh_node *node = user_data;
> -	uint16_t duration;
> +	uint16_t duration = 0;
>  	struct mesh_io *io;
>  	struct mesh_net *net;
> +	const char *key;
> +	struct l_dbus_message_iter options, var;
>  	const char *sender = l_dbus_message_get_sender(msg);
>  
>  	if (strcmp(sender, node_get_owner(node)))
>  		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
>  
> -	if (!l_dbus_message_get_arguments(msg, "q", &duration))
> +	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> +	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
> +		bool failed = true;
> +
> +		if (!strcmp(key, "Seconds")) {
> +			if (l_dbus_message_iter_get_variant(&var, "q",
> +							    &duration)) {
> +				failed = false;
> +			}
> +		}

I think failing in this in this way is not truely "Forward Compatible". If a key that is *not* Seconds is found
in the dictionary, this will always return an error.  I think it would be better if the key is ignored.

The only "fail" case should be if a key has a known valid, but has an incorrect type (not "q" in the case of
Seconds), or if the key is supported, but the value is outside the acceptable range (is there an acceptable
range for this)?

We agreed, I think, that the *non* existance of Seconds means "Unlimited".

> +
> +		if (failed)
> +			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> +							"Invalid options");
> +	}
> +
>  	if (scan_node && scan_node != node)
>  		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
>  
> @@ -752,13 +773,13 @@ static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
>  static void setup_management_interface(struct l_dbus_interface *iface)
>  {
>  	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
> -								"ay", "uuid");
> +						"aya{sv}", "uuid", "options");
>  	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
>  				"", "qyay", "primary", "count", "dev_key");
>  	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
>  						"", "qy", "primary", "count");
>  	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
> -							"", "q", "seconds");
> +							"", "a{sv}", "options");
>  	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
>  						cancel_scan_call, "", "");
>  	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,
Stotland, Inga March 27, 2020, 10:22 p.m. UTC | #2 
Hi Brian,

On Fri, 2020-03-27 at 21:17 +0000, Gix, Brian wrote:
> Hi Inga,
> On Fri, 2020-03-27 at 11:42 -0700, Inga Stotland wrote:
> > The following methods are modified to allow for future development:
> > 
> > Interface org.bluez.mesh.Management1:
> > 
> > Old: void UnprovisionedScan(uint16 seconds)
> > New: void UnprovisionedScan(dict options)
> > 
> >     The options parameter is a dictionary with the following keys defined:
> >     uint16 Seconds
> >                 Specifies number of seconds for scanning to be active.
> >                 If set to 0 or if this key is not present, then the
> >                 scanning will continue until UnprovisionedScanCancel()
> >                 or AddNode() methods are called.
> >     other keys TBD
> > 
> > Old: void AddNode(array{byte}[16] uuid)
> > New: void AddNode(array{byte}[16] uuid, dict options)
> > 
> >     The options parameter is currently an empty dictionary
> > 
> > Interface org.bluez.mesh.Provisioner1
> > 
> > Old: void ScanResult(int16 rssi, array{byte} data)
> > New: void ScanResult(int16 rssi, array{byte} data, dict options)
> > 
> >     The options parameter is currently an empty dictionary
> > ---
> >  mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/mesh/manager.c b/mesh/manager.c
> > index 0909c7e16..8e948e47d 100644
> > --- a/mesh/manager.c
> > +++ b/mesh/manager.c
> > @@ -217,21 +217,22 @@ static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
> >  						void *user_data)
> >  {
> >  	struct mesh_node *node = user_data;
> > -	struct l_dbus_message_iter iter_uuid;
> > +	struct l_dbus_message_iter iter_uuid, options;
> >  	struct l_dbus_message *reply;
> >  	uint8_t *uuid;
> > -	uint32_t n;
> > +	uint32_t n = 22;
> >  
> >  	l_debug("AddNode request");
> >  
> > -	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
> > +	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >  
> >  	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
> > -								|| n != 16)
> > +	    || n != 16) {
> > +		l_debug("n = %u", n);
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> >  							"Bad device UUID");
> > -
> > +	}
> >  	/* Allow AddNode to cancel Scanning if from the same node */
> >  	if (scan_node) {
> >  		if (scan_node != node)
> > @@ -361,6 +362,9 @@ static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
> >  	builder = l_dbus_message_builder_new(msg);
> >  	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
> >  	dbus_append_byte_array(builder, data + 2, len -2);
> > +	l_dbus_message_builder_enter_array(builder, "{sv}");
> > +	/* TODO: populate with options when defined */
> > +	l_dbus_message_builder_leave_array(builder);
> >  	l_dbus_message_builder_finalize(builder);
> >  	l_dbus_message_builder_destroy(builder);
> >  
> > @@ -372,17 +376,34 @@ static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
> >  						void *user_data)
> >  {
> >  	struct mesh_node *node = user_data;
> > -	uint16_t duration;
> > +	uint16_t duration = 0;
> >  	struct mesh_io *io;
> >  	struct mesh_net *net;
> > +	const char *key;
> > +	struct l_dbus_message_iter options, var;
> >  	const char *sender = l_dbus_message_get_sender(msg);
> >  
> >  	if (strcmp(sender, node_get_owner(node)))
> >  		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
> >  
> > -	if (!l_dbus_message_get_arguments(msg, "q", &duration))
> > +	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >  
> > +	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
> > +		bool failed = true;
> > +
> > +		if (!strcmp(key, "Seconds")) {
> > +			if (l_dbus_message_iter_get_variant(&var, "q",
> > +							    &duration)) {
> > +				failed = false;
> > +			}
> > +		}
> 
> I think failing in this in this way is not truely "Forward Compatible". If a key that is *not* Seconds is found
> in the dictionary, this will always return an error.  I think it would be better if the key is ignored.
> 
> The only "fail" case should be if a key has a known valid, but has an incorrect type (not "q" in the case of
> Seconds), or if the key is supported, but the value is outside the acceptable range (is there an acceptable
> range for this)?
> 

"Forward compatible" means that further keys shall be documented in
mesh-api.txt and then correctly processed by the daemon.
I would prefer to fail on unrecognized keywords.

> We agreed, I think, that the *non* existance of Seconds means "Unlimited".

Either 0 or absense of 'Seconds' keyword mean continuous scan.

> 
> > +
> > +		if (failed)
> > +			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> > +							"Invalid options");
> > +	}
> > +
> >  	if (scan_node && scan_node != node)
> >  		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
> >  
> > @@ -752,13 +773,13 @@ static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
> >  static void setup_management_interface(struct l_dbus_interface *iface)
> >  {
> >  	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
> > -								"ay", "uuid");
> > +						"aya{sv}", "uuid", "options");
> >  	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
> >  				"", "qyay", "primary", "count", "dev_key");
> >  	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
> >  						"", "qy", "primary", "count");
> >  	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
> > -							"", "q", "seconds");
> > +							"", "a{sv}", "options");
> >  	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
> >  						cancel_scan_call, "", "");
> >  	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,
diff mbox series

Patch

diff --git a/mesh/manager.c b/mesh/manager.c
index 0909c7e16..8e948e47d 100644
--- a/mesh/manager.c
+++ b/mesh/manager.c
@@ -217,21 +217,22 @@  static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
 						void *user_data)
 {
 	struct mesh_node *node = user_data;
-	struct l_dbus_message_iter iter_uuid;
+	struct l_dbus_message_iter iter_uuid, options;
 	struct l_dbus_message *reply;
 	uint8_t *uuid;
-	uint32_t n;
+	uint32_t n = 22;
 
 	l_debug("AddNode request");
 
-	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
+	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
 
 	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
-								|| n != 16)
+	    || n != 16) {
+		l_debug("n = %u", n);
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
 							"Bad device UUID");
-
+	}
 	/* Allow AddNode to cancel Scanning if from the same node */
 	if (scan_node) {
 		if (scan_node != node)
@@ -361,6 +362,9 @@  static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
 	builder = l_dbus_message_builder_new(msg);
 	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
 	dbus_append_byte_array(builder, data + 2, len -2);
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+	/* TODO: populate with options when defined */
+	l_dbus_message_builder_leave_array(builder);
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
@@ -372,17 +376,34 @@  static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
 						void *user_data)
 {
 	struct mesh_node *node = user_data;
-	uint16_t duration;
+	uint16_t duration = 0;
 	struct mesh_io *io;
 	struct mesh_net *net;
+	const char *key;
+	struct l_dbus_message_iter options, var;
 	const char *sender = l_dbus_message_get_sender(msg);
 
 	if (strcmp(sender, node_get_owner(node)))
 		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
 
-	if (!l_dbus_message_get_arguments(msg, "q", &duration))
+	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
 
+	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
+		bool failed = true;
+
+		if (!strcmp(key, "Seconds")) {
+			if (l_dbus_message_iter_get_variant(&var, "q",
+							    &duration)) {
+				failed = false;
+			}
+		}
+
+		if (failed)
+			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
+							"Invalid options");
+	}
+
 	if (scan_node && scan_node != node)
 		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
 
@@ -752,13 +773,13 @@  static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
 static void setup_management_interface(struct l_dbus_interface *iface)
 {
 	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
-								"ay", "uuid");
+						"aya{sv}", "uuid", "options");
 	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
 				"", "qyay", "primary", "count", "dev_key");
 	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
 						"", "qy", "primary", "count");
 	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
-							"", "q", "seconds");
+							"", "a{sv}", "options");
 	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
 						cancel_scan_call, "", "");
 	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,