Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201006171333.BlueZ.v6.1.I2830b9c1212a64b062201ed9f2b71294f50ad22d@changeid>
 <20201006171333.BlueZ.v6.2.I578ae5e76fcf7243206a27d4f5a25783662a5f14@changeid>
 <CABBYNZ+pmByP5WMibCGQJXTvy7ephJdJdALg=mYagzAjWMLk9w@mail.gmail.com> <CABmPvSE17drmxua9MopjH1yYS0-9Lu0UL9wmcFXCpMC06vC99A@mail.gmail.com>
In-Reply-To: <CABmPvSE17drmxua9MopjH1yYS0-9Lu0UL9wmcFXCpMC06vC99A@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 13 Oct 2020 16:45:24 -0700
Message-ID: <CABBYNZJpJneZED3wY7yQvKeHKF2HM2KmJ3UYL9O+GAA=yhu3Vg@mail.gmail.com>
Subject: Re: [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on
 stored monitors
To:     Miao-chen Chou <mcchou@chromium.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>,
        Howard Chung <howardchung@google.com>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Alain Michaud <alainm@chromium.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Manish Mandlik <mmandlik@chromium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Miao-chen,

On Mon, Oct 12, 2020 at 2:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> I did think of adding pattern to bt_ad at the beginning, but here are
> reasons why I ended up with hosting the definition of pattern in
> adv_monitor.
> (1)  Pattern is specific to monitoring purpose. An advertisement
> should not include patterns as its fields due to that fact that a
> pattern hosts an offset. So I wasn't sure about its justification to
> be placed in shared/ad. But if you foresee that it would be reused,
> then I am more than happy to add it to shared/ad.
> (2) Introducing helpers as you suggested below indeed make it more
> unittestable. However, it also implied that EIR data (this is in fact
> AD data) needs to be parsed into a new bt_ad for pattern comparison,
> and I didn't see an obvious benefit of converting EIR data into a
> bt_ad just for the comparison.

Regarding (1) like I said you will need another struct to store the
offset and length for doing the pattern matching but otherwise it is
pretty similar, regarding (2) there is actually a real benefit that we
could in the future drop the eir_ helpers and unit test just the bt_ad
including pattern matching as well.

> Maybe we can add a struct bt_ad_pattern in shared/ad.h and introduce
> the following two functions. What do you think?
>
> struct bt_ad_pattern *bt_ad_pattern_new(uint8_t type, size_t offset,
> size_t len, const void *data);

Not sure I follow you here, that doesn't seem to be about the parsing
of the AD but the creation of an object for matching, not sure we need
that since you can probably just pass a stack variable directly, in
any case you would need a free as well.

> /* |data| is one single AD data field so that we can avoid converting
> EIR data to bt_ad */
> bool bt_ad_pattern bt_ad_pattern_match(struct bt_ad_pattern *pattern,
> void *data, size_t len);

You may have more than one entry of the same type, anyway if we do
have something like bt_ad_new_with_data that means we can pass it
around and not have to reparse it when doing custom filtering. We
might as well have a callback that does get called every time there is
a match, also it may be possible to pass all the pattern at once in
which case we can pass the pattern which matched along with the data
thus having the call it multiple times for each monitor.