Re: [PATCH BlueZ v4 09/30] mesh: Acceptor side provisioning implimentation

[Marcel Holtmann <marcel@holtmann.org>]

Hi Brian,

> ---
> mesh/prov-acceptor.c | 711 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 711 insertions(+)
> create mode 100644 mesh/prov-acceptor.c
> 
> diff --git a/mesh/prov-acceptor.c b/mesh/prov-acceptor.c
> new file mode 100644
> index 000000000..ac42e1e2c
> --- /dev/null
> +++ b/mesh/prov-acceptor.c
> @@ -0,0 +1,711 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2018  Intel Corporation. All rights reserved.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <sys/select.h>
> +#include <sys/time.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <termios.h>
> +
> +#include <ctype.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <getopt.h>
> +#include <time.h>
> +#include <ell/ell.h>
> +
> +#include "mesh/mesh-defs.h"
> +#include "src/shared/ecc.h"
> +
> +#include "mesh/util.h"
> +#include "mesh/net_keys.h"
> +#include "mesh/crypto.h"
> +#include "mesh/net.h"
> +#include "mesh/error.h"
> +#include "mesh/prov.h"
> +#include "mesh/provision.h"
> +#include "mesh/pb-adv.h"
> +#include "mesh/mesh.h"
> +#include "mesh/agent.h"
> +
> +/* Quick size sanity check */
> +static const uint16_t expected_pdu_size[] = {
> +	2,	/* PROV_INVITE */
> +	12,	/* PROV_CAPS */
> +	6,	/* PROV_START */
> +	65,	/* PROV_PUB_KEY */
> +	1,	/* PROV_INP_CMPLT */
> +	17,	/* PROV_CONFIRM */
> +	17,	/* PROV_RANDOM */
> +	34,	/* PROV_DATA */
> +	1,	/* PROV_COMPLETE */
> +	2,	/* PROV_FAILED */
> +};
> +
> +#define BEACON_TYPE_UNPROVISIONED		0x00
> +
> +static const uint8_t pkt_filter = MESH_AD_TYPE_PROVISION;
> +static const uint8_t bec_filter[] = {MESH_AD_TYPE_BEACON,
> +						BEACON_TYPE_UNPROVISIONED};
> +
> +enum acp_state {
> +	ACP_PROV_IDLE = 0,
> +	ACP_PROV_CAPS_SENT,
> +	ACP_PROV_CAPS_ACKED,
> +	ACP_PROV_KEY_SENT,
> +	ACP_PROV_KEY_ACKED,
> +	ACP_PROV_INP_CMPLT_SENT,
> +	ACP_PROV_INP_CMPLT_ACKED,
> +	ACP_PROV_CONF_SENT,
> +	ACP_PROV_CONF_ACKED,
> +	ACP_PROV_RAND_SENT,
> +	ACP_PROV_RAND_ACKED,
> +	ACP_PROV_CMPLT_SENT,
> +	ACP_PROV_FAIL_SENT,
> +};
> +
> +#define MAT_REMOTE_PUBLIC	0x01
> +#define MAT_LOCAL_PRIVATE	0x02
> +#define MAT_RAND_AUTH		0x04
> +#define MAT_SECRET	(MAT_REMOTE_PUBLIC | MAT_LOCAL_PRIVATE)
> +
> +struct mesh_prov_acceptor {
> +	mesh_prov_acceptor_complete_func_t cmplt;
> +	prov_trans_tx_t trans_tx;
> +	void *agent;
> +	void *caller_data;
> +	void *trans_data;
> +	struct l_timeout *timeout;
> +	uint32_t to_secs;
> +	enum acp_state	state;
> +	uint8_t transport;
> +	uint8_t material;
> +	uint8_t expected;
> +	int8_t previous;
> +	struct conf_input conf_inputs;
> +	uint8_t calc_key[16];
> +	uint8_t salt[16];
> +	uint8_t confirm[16];
> +	uint8_t s_key[16];
> +	uint8_t s_nonce[13];
> +	uint8_t private_key[32];
> +	uint8_t secret[32];
> +	uint8_t rand_auth_workspace[48];
> +};
> +
> +static struct mesh_prov_acceptor *prov = NULL;
> +
> +static void acceptor_free(void)
> +{
> +
> +	if (prov)
> +		l_timeout_remove(prov->timeout);
> +
> +	mesh_send_cancel(bec_filter, sizeof(bec_filter));
> +	mesh_send_cancel(&pkt_filter, sizeof(pkt_filter));
> +
> +	if (prov->trans_tx) {
> +		if (prov->transport == PB_ADV)
> +			pb_adv_unreg(prov->trans_data);
> +#if defined(GATT_ENABLED)
> +		/* TODO: Cleanup GATT bearer if exists */
> +		else if (prov->transport == PB_GATT)
> +			pb_gatt_unreg(prov->trans_data);
> +#endif

this is a simple no. Please stop hacking the code like yes. And yes, adding ifdef is hacking it. Either you abstract different bearer cleanly or they will be all always available. I am not doing two test runs to check your idea of build options.

Regards

Marcel