Date:   Wed, 17 Apr 2019 07:51:32 +0200
From:   =?utf-8?Q?Micha=C5=82?= Lowas-Rzechonek 
        <michal.lowas-rzechonek@silvair.com>
To:     Brian Gix <brian.gix@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, inga.stotland@intel.com,
        marcel@holtmann.org, johan.hedberg@gmail.com
Subject: Re: [PATCH BlueZ v5 1/1] mesh: Add APIs for Provisioner and Config
 Client
Message-ID: <20190417055132.scxvzhusx2suasdv@kynes>
Mail-Followup-To: Brian Gix <brian.gix@intel.com>,
        linux-bluetooth@vger.kernel.org, inga.stotland@intel.com,
        marcel@holtmann.org, johan.hedberg@gmail.com
References: <20190415194946.13121-1-brian.gix@intel.com>
 <20190415194946.13121-2-brian.gix@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190415194946.13121-2-brian.gix@intel.com>
User-Agent: NeoMutt/20180716
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

First of all, thank you for taking my comments into consideration!

On 04/15, Brian Gix wrote:
> +	 uint64 token ImportLocalNode(string config_file)

I am somewhat uncomfortable with passing a file path here. The caller
would need to create a temporary file, which is a little cumbersome, and
might fail if the daemon is running on another machine.

Not sure what are the size constraints (if any), but I think it might be
better to pass the JSON as a string.

> +	void AddNetKey(object element_path, uint16 destination,
> +			uint16 subnet_index, uint16 net_index, boolean update)
> (...)
> +	void AddAppKey(object element_path, uint16 destination,
> +			uint16 app_index, uint16 net_index, boolean update)

If I understand correctly, these are convenience functions for
SendWithDeviceKey, so that the application wouldn't need to construct
access payloads for configuration messages?

If so, I guess we will need more functions like that, ideally to cover
all messages mentioned in section 4.3.4 of the mesh profile?

I would suggest a few things:
 - name these functions in the same way as the corresponding message
 - possibly extract these into org.bluez.mesh.ConfigClient1 and
   org.bluez.mesh.HealthClient1 interfaces, so that we don't need to
   name functions "ConfigAppKeyApp", just "AppKeyApp"
 - possibly add corresponding "* Status()" calls to application
   hierarchy; see the question below about DevKeyMessageReceived

> +	void ImportRemoteNode(uint16 primary, uint8 count,
> +					array{byte}[16] device_key)
> +
> +		This method is used by the application to import a remote node
> +		that has been provisioned by an external process.
> +
> +		The primary parameter specifies the unicast address of the
> +		the node being imported.
> +
> +		The count parameter specifies the number of elements that are
> +		assigned to this remote node.
> +
> +		The device_key parameter is the access layer key that will be
> +		will used to decrypt privledged messages from this remote node.
> +
> +		PossibleErrors:
> +			org.bluez.mesh.Error.InvalidArguments
> +

> +	void DeleteRemoteNode(uint16 primary, uint8 count)
> (...)
> +		The count parameter specifies the number of elements that were
> +		assigned to the remote node.

Do we need the count here? I think the daemon knows that already.

> +	void SendWithDeviceKey(object element_path, uint16 destination,
> +					uint16 net_index, array{byte} data)
> +
> +		This method is used to send a message originated by a local
> +		model encoded with the device key of the remote node.
> (...)
> +	void DevKeyMessageReceived(uint16 source, uint16 net_index,
> +							array{byte} data)
> +
> +		This method is called by meshd daemon when a message arrives
> +		addressed to the application, which was sent with the remote
> +		node's device key.

Correct me if I'm wrong, but it seems that these two are not required to
support foundation models?

IIRC, requests coming into the daemon from a remote Config Client are
handled internally and then passed to the application via
UpdateModelConfiguation() and friends, while outgoing requests are to be
handled by AddNetKey(), etc.?

On the other hand, nodes might implement vendor models using device
security, so I guess these functions are meant for this use case?

Also, I would suggest a small rename to keep things a bit more
consistent:
 - if SendWithDeviceKey(), then 'MessageReceivedWithDeviceKey()
 - if DevKeyMessageRedeived(), then 'DevKeySend()

but in general, I'd rather rename the original Send/MessageReceived, so
that we end up with:
 - SendApplicationMessage + ApplicationMessageReceived
 - SendDeviceMessage + DeviceMessageReceived
 - SendControlMessage + ControlMessageReceived (but I don't see a need
   to expose this to application, at least not at the moment)


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