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

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

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,