Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH BlueZ] mesh: Add APIs for Provisioner and Config Client
From:   Johan Hedberg <johan.hedberg@gmail.com>
In-Reply-To: <20190313221526.16890-1-brian.gix@intel.com>
Date:   Thu, 14 Mar 2019 09:41:01 +0200
Cc:     linux-bluetooth@vger.kernel.org, inga.stotland@intel.com,
        Marcel Holtmann <marcel@holtmann.org>,
        Luiz Von Dentz <luiz.dentz@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <C54E4ABE-6257-486E-965C-EAA5F13F3402@gmail.com>
References: <20190313221526.16890-1-brian.gix@intel.com>
To:     Brian Gix <brian.gix@intel.com>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

On 14 Mar 2019, at 0.15, Brian Gix <brian.gix@intel.com> wrote:
> +	uint64 token, array{byte}[16] device_key
> +			CreateNetwork(object app_root, array{byte}[16] =
uuid,
> +			array{byte}[16] net_key)
> +		This is the first method that an application calls to =
become

Minor: please be consistent with keeping an empty line between the =
function signature and its description. There are a few more places in =
this document with the same issue (which I won=E2=80=99t point out =
separately).

> +	array{byte} DecryptWithDeviceKey(array{byte}[16] key,
> +			array{byte}[13] nonce, array{byte} =
encrypted_data)

It seems inefficient to have a design requiring this kind of roundtrips =
between mesh and the application. Why can=E2=80=99t we have meshd manage =
the device key for remote devices (and also keep track of which =
application has access to them)? IMO we should keep the persistent =
storage requirements of the application to the bare minimum and put that =
burden mostly on the meshd side.

> +Mesh Provisioning Hierarchy
> +	void AddNode(array{byte}[16] uuid)

Wouldn=E2=80=99t =E2=80=9CProvision=E2=80=9D be a more matching name for =
this method?

> +Mesh Provisioner Hierarchy
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> +Service		unique name
> +Interface	org.bluez.mesh.Provisioner1
> +Object path	freely definable
> +
> +	int8 rssi, array{byte} data ScanResult()

This looks backward - shouldn=E2=80=99t the parameters be input =
parameters and not return parameters? As return parameters this method =
doesn=E2=80=99t make any sense to me.

> +	void AddNodeComplete(array{byte}[16] uuid, uint16 unicast, uint8 =
count)

This would be ProvisioningComplete if you follow my earlier naming =
suggestion.

> +	void AddNodeFailed(array{byte}[16] uuid, string reason)

And ProvisioningFailed here.

Johan=