Hi Daniel,
On Wed, Sep 16, 2020 at 4:25 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> wrote:
>
> This change adds parsers for the advertising intervals and tx power
> properties of the LEAdvertisement1 object. It validates that each field
> adheres to the 5.2 spec, and that min and max intervals are compatible
> with each other, i.e. that min interval is less than max interval.
>
> A note here for maintainers: The tx power that is sent in the hci
> parameter command is an int8_t, but as far as I can tell, there is no
> clean way to use a signed 8-bit integer in dbus. The dbus byte type
> seems incompatible with negative values in high-level languages (python)
> without awkward usage manipulation on the client side. For this reason,
> I chose to use an int16_t type for the tx power dbus field, which is
> then downcasted to the int8_t in bluetoothd, which at least makes the
> signed-ness of the type crystal clear to the dbus client that uses it.
>
> This change is manually verified by ensuring the intervals and tx power
> parameters are correctly parsed from the LEAdvertisement1 object, and
> that the parse fails if the parameters are incorrect or not compatible
> with each other.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx>
> ---
>
> src/advertising.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 172a83907..82ee87313 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -63,6 +63,11 @@ struct btd_adv_manager {
> #define AD_TYPE_BROADCAST 0
> #define AD_TYPE_PERIPHERAL 1
>
> +/* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2585
> + * defines tx power value indicating no preference
> + */
> +#define ADV_TX_POWER_NO_PREFERENCE 0x7F
> +
> struct btd_adv_client {
> struct btd_adv_manager *manager;
> char *owner;
> @@ -83,6 +88,9 @@ struct btd_adv_client {
> struct bt_ad *data;
> struct bt_ad *scan;
> uint8_t instance;
> + uint32_t min_interval;
> + uint32_t max_interval;
> + int8_t tx_power;
> };
>
> struct dbus_obj_match {
> @@ -946,6 +954,74 @@ static bool parse_secondary(DBusMessageIter *iter,
> return false;
> }
>
> +static bool parse_min_interval(DBusMessageIter *iter,
> + struct btd_adv_client *client)
> +{
> + if (!iter)
> + return false;
> +
> + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT32)
> + return false;
> +
> + dbus_message_iter_get_basic(iter, &client->min_interval);
> +
> + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2584
> + * defines acceptable interval range
> + */
> + if (client->min_interval < 0x20 || client->min_interval > 0xFFFFFF) {
> + client->min_interval = 0;
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static bool parse_max_interval(DBusMessageIter *iter,
> + struct btd_adv_client *client)
> +{
> + if (!iter)
> + return false;
> +
> + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT32)
> + return false;
> +
> + dbus_message_iter_get_basic(iter, &client->max_interval);
> +
> + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2584
> + * defines acceptable interval range
> + */
> + if (client->max_interval < 0x20 || client->max_interval > 0xFFFFFF) {
> + client->max_interval = 0;
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static bool parse_tx_power(DBusMessageIter *iter,
> + struct btd_adv_client *client)
> +{
> + int16_t val;
> +
> + if (!iter)
> + return false;
> +
> + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT16)
> + return false;
> +
> + dbus_message_iter_get_basic(iter, &val);
> +
> + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2585
> + * defines acceptable tx power range
> + */
> + if (val < -127 || val > 20)
> + return false;
> +
> + client->tx_power = val;
> +
> + return true;
> +}
> +
> static struct adv_parser {
> const char *name;
> bool (*func)(DBusMessageIter *iter, struct btd_adv_client *client);
> @@ -964,6 +1040,9 @@ static struct adv_parser {
> { "Discoverable", parse_discoverable },
> { "DiscoverableTimeout", parse_discoverable_timeout },
> { "SecondaryChannel", parse_secondary },
> + { "MinInterval", parse_min_interval },
> + { "MaxInterval", parse_max_interval },
We will need to these to the D-Bus documentation if you want to extend it.
> + { "TxPower", parse_tx_power },
TxPower is already part of the include, or you want the ability to set
an arbitrary TxPower?
> { },
> };
>
> @@ -1092,6 +1171,13 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
> goto fail;
> }
>
> + if (client->min_interval > client->max_interval) {
> + /* Min interval must not be bigger than max interval */
> + error("MinInterval must be less than MaxInterval (%lu > %lu)",
> + client->min_interval, client->max_interval);
> + goto fail;
> + }
> +
> err = refresh_adv(client, add_adv_callback, &client->add_adv_id);
> if (!err)
> return NULL;
> @@ -1167,6 +1253,9 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
>
> client->manager = manager;
> client->appearance = UINT16_MAX;
> + client->tx_power = ADV_TX_POWER_NO_PREFERENCE;
> + client->min_interval = 0;
> + client->max_interval = 0;
>
> return client;
>
> --
> 2.28.0.618.gf4bc123cb7-goog
>
--
Luiz Augusto von Dentz
