Hi Luiz,
On Tue, Jun 21, 2022 at 1:57 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Mike,
>
> On Mon, Jun 13, 2022 at 3:28 PM Mike Brudevold <puffy.taco@xxxxxxxxx> wrote:
> >
> > Hi Luiz,
> >
> > On Mon, Jun 13, 2022 at 4:55 PM Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx> wrote:
> > >
> > > Hi Mike,
> > >
> > > On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@xxxxxxxxx> 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?
If you have any, I welcome them. I have an idea of what I would do to
augment struct bt_ad by making bt_ad_new_with_data smarter to parse
out to other queues/data values, but it hasn't been the highest
priority so I haven't put any time into it.
>
> > >
> > > > 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
