Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220610221124.18591-1-puffy.taco@gmail.com> <20220610221124.18591-2-puffy.taco@gmail.com>
 <CABBYNZKZ6jHeQMO3r_NC1phA03Vg67o9dejKSGpJ1i9LCq_aOQ@mail.gmail.com> <CAB7rCTg4+gmzD3emRB6rxfo7RiaJsU+4oBWVQBEut3nr-WqqRA@mail.gmail.com>
In-Reply-To: <CAB7rCTg4+gmzD3emRB6rxfo7RiaJsU+4oBWVQBEut3nr-WqqRA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 13 Jun 2022 14:55:02 -0700
Message-ID: <CABBYNZJi+eQq=k_okUiPWr4vLr_Ehdrfq_RkSrkUqGKCeNPhng@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing
To:     Mike Brudevold <puffy.taco@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Michael Brudevold <michael.brudevold@veranexsolutions.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mike,

On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
>
> Hi Luiz,
>
> > It might be better to handle this via bt_ad instance instead of
> > eir_data, in fact the plan was always to switch to bt_ad but it seems
> > we forgot about it at some point.
>
> Are you thinking something like below (doesn't fully compile,
> name2utf8 is static in eir so I did nothing about that yet)?
> Basically where the ad code parses the EIR data, but the neard plugin
> still manages what to do with the data?  The alternative would be
> where device.c became smarter to consume the ad struct itself and the
> neard plugin becomes a simple conduit for the ad data.

The later is probably better alternative, like I said I wrote bt_ad to
replace eir handling completely so we could also write proper unit
testing for it, Im fine if you want to take the time to change the
daemon core itself but at least introduce support for the types you
will be using in the plugin and then make use of them.

> index 77a4630da..3d4064515 100644
> --- a/plugins/neard.c
> +++ b/plugins/neard.c
> @@ -31,6 +31,7 @@
>  #include "src/agent.h"
>  #include "src/btd.h"
>  #include "src/shared/util.h"
> +#include "src/shared/ad.h"
>
>  #define NEARD_NAME "org.neard"
>  #define NEARD_PATH "/org/neard"
> @@ -336,6 +337,52 @@ static int check_device(struct btd_device *device)
>         return 0;
>  }
>
> +static void process_oob_adv(void *data, void *user_data)
> +{
> +       struct bt_ad_data *ad_data = data;
> +       struct oob_params *remote = user_data;
> +       uint8_t name_len;
> +
> +       switch (ad_data->type) {
> +       case EIR_NAME_SHORT:
> +       case EIR_NAME_COMPLETE:
> +               name_len = ad_data->len;
> +
> +               /* Some vendors put a NUL byte terminator into
> +                       * the name */
> +               while (name_len > 0 && ad_data->data[name_len - 1] == '\0')
> +                       name_len--;
> +
> +               g_free(remote->name);
> +
> +               remote->name = name2utf8(ad_data->data, name_len);
> +               break;
> +
> +       case BT_AD_LE_DEVICE_ADDRESS:
> +               if (ad_data->len < sizeof(bdaddr_t) + 1)
> +                       break;
> +
> +               memcpy(&remote->address, ad_data->data, sizeof(bdaddr_t));
> +               remote->address_type = ad_data->data[sizeof(bdaddr_t)] & 0x1 ?
> +                               BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
> +               break;
> +
> +       case EIR_LE_SC_CONF:
> +               if (ad_data->len < 16)
> +                       break;
> +               free(remote->hash256);
> +               remote->hash256 = util_memdup(ad_data->data, 16);
> +               break;
> +
> +       case EIR_LE_SC_RAND:
> +               if (ad_data->len < 16)
> +                       break;
> +               free(remote->randomizer256);
> +               remote->randomizer256 = util_memdup(ad_data->data, 16);
> +               break;
> +       }
> +}

Do we need to duplicate this information? I'd consider just using the
bt_ad instance to parse and store them, well perhaps we want to
introduce something like bt_ad_foreach_type so you can locate the data
by type?

>  static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
>  {
>         struct eir_data eir_data;
> @@ -370,32 +417,17 @@ static int process_eir(uint8_t *eir, size_t
> size, struct oob_params *remote)
>
>  static void process_eir_le(uint8_t *eir, size_t size, struct
> oob_params *remote)
>  {
> -       struct eir_data eir_data;
> +       struct bt_ad *ad;
>
>         DBG("size %zu", size);
>
> -       memset(&eir_data, 0, sizeof(eir_data));
> -
> -       eir_parse(&eir_data, eir, size);
> -
> -       bacpy(&remote->address, &eir_data.addr);
> -       remote->address_type = eir_data.addr_type;
> -
> -       remote->class = eir_data.class;
> -
> -       remote->name = eir_data.name;
> -       eir_data.name = NULL;
> -
> -       remote->services = eir_data.services;
> -       eir_data.services = NULL;
> -
> -       remote->hash256 = eir_data.hash256;
> -       eir_data.hash256 = NULL;
> +       ad = bt_ad_new_with_data(size, eir);
>
> -       remote->randomizer256 = eir_data.randomizer256;
> -       eir_data.randomizer256 = NULL;
> +       if (ad) {
> +               bt_ad_foreach_data(ad, process_oob_adv, remote);
>
> -       eir_data_free(&eir_data);
> +               bt_ad_unref(ad);
> +       }
>  }


-- 
Luiz Augusto von Dentz