Re: [PATCH BlueZ 2/4] core/advertising: Parse IncludeTXPower

[Szymon Janc <szymon.janc@tieto.com>]

Hi Michael,

On Friday 10 of April 2015 12:06:06 Michael Janssen wrote:
> Parse the IncludeTXPower property of the advertisement object, and
> pass the appropriate flag to MGMT if it is set.
> 
> Uses MGMT Read Advertising Features Command to determine the maximum
> length allowed.
> ---
>  src/advertising.c | 79
> +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 74
> insertions(+), 5 deletions(-)
> 
> diff --git a/src/advertising.c b/src/advertising.c
> index 2f8e539..8acd5b4 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -46,6 +46,7 @@ struct btd_advertising {
>  	struct queue *ads;
>  	struct mgmt *mgmt;
>  	uint16_t mgmt_index;
> +	uint8_t max_adv_len;
>  };
> 
>  #define AD_TYPE_BROADCAST 0
> @@ -59,6 +60,7 @@ struct advertisement {
>  	GDBusProxy *proxy;
>  	DBusMessage *reg;
>  	uint8_t type; /* Advertising type */
> +	bool include_tx_power;
>  	struct bt_ad *data;
>  	uint8_t instance;
>  };
> @@ -361,6 +363,22 @@ fail:
>  	return false;
>  }
> 
> +static bool parse_advertising_include_tx_power(GDBusProxy *proxy,
> +							bool *included)
> +{
> +	DBusMessageIter iter;
> +
> +	if (!g_dbus_proxy_get_property(proxy, "IncludeTXPower", &iter))
> +		return true;
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_BOOLEAN)
> +		return false;
> +
> +	dbus_message_iter_get_basic(&iter, included);

Due to historical reasons dbus_bool_t is 4 bytes long (check dbus-types.h)
To be on a safe side I'd do:

dbus_bool_t b;

dbus_message_iter_get_basic(&iter, &b);

*included = b;


> +
> +	return true;
> +}
> +
>  static void add_advertising_callback(uint8_t status, uint16_t length,
>  					  const void *param, void *user_data)
>  {
> @@ -375,19 +393,44 @@ static void add_advertising_callback(uint8_t status,
> uint16_t length, ad->instance = rp->instance;
>  }
> 
> +static size_t calc_max_adv_len(struct advertisement *ad,
> +								uint32_t flags)

nitpick: this should fit in single line

> +{
> +	size_t max = ad->manager->max_adv_len;
> +
> +	if (flags & MGMT_ADV_FLAG_TX_POWER)
> +		max -= 3;

At least comment on where those 3/4 bytes came from.

> +
> +	if (flags & (MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV |
> +						MGMT_ADV_FLAG_MANAGED_FLAGS))
> +		max -= 3;
> +
> +	if (flags & MGMT_ADV_FLAG_APPEARANCE)
> +		max -= 4;
> +
> +	return max;
> +}
> +
>  static DBusMessage *refresh_advertisement(struct advertisement *ad)
>  {
>  	struct mgmt_cp_add_advertising *cp;
>  	uint8_t param_len;
>  	uint8_t *adv_data;
>  	size_t adv_data_len;
> +	uint32_t flags = 0;
> 
>  	DBG("Refreshing advertisement: %s", ad->path);
> 
> +	if (ad->type == AD_TYPE_PERIPHERAL)
> +		flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
> +
> +	if (ad->include_tx_power)
> +		flags |= MGMT_ADV_FLAG_TX_POWER;
> +
>  	adv_data = bt_ad_generate(ad->data, &adv_data_len);
> 
> -	if (!adv_data) {
> -		error("Advertising data couldn't be generated.");
> +	if (!adv_data || (adv_data_len > calc_max_adv_len(ad, flags))) {
> +		error("Advertising data too long or couldn't be generated.");
> 
>  		return g_dbus_create_error(ad->reg, ERROR_INTERFACE
>  						".InvalidLength",
> @@ -406,9 +449,7 @@ static DBusMessage *refresh_advertisement(struct
> advertisement *ad) return btd_error_failed(ad->reg, "Failed");
>  	}
> 
> -	if (ad->type == AD_TYPE_PERIPHERAL)
> -		cp->flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
> -
> +	cp->flags = flags;
>  	cp->instance = ad->instance;
>  	cp->adv_data_len = adv_data_len;
>  	memcpy(cp->data, adv_data, adv_data_len);
> @@ -457,6 +498,12 @@ static DBusMessage *parse_advertisement(struct
> advertisement *ad) goto fail;
>  	}
> 
> +	if (!parse_advertising_include_tx_power(ad->proxy,
> +						&ad->include_tx_power)) {
> +		error("Property \"IncludeTXPower\" failed to parse");
> +		goto fail;
> +	}
> +
>  	return refresh_advertisement(ad);
> 
>  fail:
> @@ -636,6 +683,20 @@ static void advertising_manager_destroy(void
> *user_data) free(manager);
>  }
> 
> +static void read_adv_features_callback(uint8_t status, uint16_t length,
> +					const void *param, void *user_data)
> +{
> +	struct btd_advertising *manager = user_data;
> +	const struct mgmt_rp_read_adv_features *feat = param;
> +
> +	if (status || !param) {
> +		error("Failed to read advertising features");

I'd print status error here as well.

> +		return;
> +	}

Usually we check if daemon got enough bytes in response before accessing it:

if (length < sizeof(*rp)) {
         error("Wrong size of read adv features response");
         return;
}

> +
> +	manager->max_adv_len = feat->max_adv_data_len;
> +}
> +
>  static struct btd_advertising *
>  advertising_manager_create(struct btd_adapter *adapter)
>  {
> @@ -657,6 +718,14 @@ advertising_manager_create(struct btd_adapter *adapter)
> 
>  	manager->mgmt_index = btd_adapter_get_index(adapter);
> 
> +	if (!mgmt_send(manager->mgmt, MGMT_OP_READ_ADV_FEATURES,
> +				manager->mgmt_index, 0, NULL,
> +				read_adv_features_callback, manager, NULL)) {
> +		error("Cannot read advertising features, MGMT version too low");

I'm not sure if you get unsupported failure here since that needs to come from 
kernel in response. So this error message doesn't seem right.


> +		advertising_manager_destroy(manager);
> +		return NULL;
> +	}
> +
>  	if (!g_dbus_register_interface(btd_get_dbus_connection(),
>  						adapter_get_path(adapter),
>  						LE_ADVERTISING_MGR_IFACE,

-- 
BR
Szymon Janc