Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Mike,

On Mon, Jun 13, 2022 at 3:28 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
>
> Hi Luiz,
>
> On Mon, Jun 13, 2022 at 4:55 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > 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?
>
> That's partly what I was checking on.  The ad code doesn't have much
> functionality right now to be able to parse the meaning of the data,
> beyond storing them in TLV format (bt_ad_data).  Which is the opposite
> to how the data is given to ad if you're creating an advertisement
> (e.g., service UUIDs are stored in bt_uuid_t format inside the service
> queue when using bt_ad_add_service_uuid, not in the data queue).  This
> means the ad code likely has to get smarter, but I wanted to make sure
> I wasn't missing something that should have been obvious.  If the ad
> code can present the data back in a usable format, then it wouldn't
> really have to be duplicated.  Otherwise this code would have been an
> easy way to not use the eir code while the ad code gets developed.
> With some concern still that there's a type_reject_list related to the
> data ad queue, but that can be completely bypassed when using
> bt_ad_new_with_data - so this method is doing something that seems
> unintended.

Are you missing some feedback on these changes?

> >
> > >  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



-- 
Luiz Augusto von Dentz