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

["Stotland, Inga" <inga.stotland@intel.com>]

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,