| Message ID | 20210727195057.Bluez.v1.1.I20397b8350f98567b8d52b895442c768250a6ab3@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Bluez,v1] gatt-db: fix service in range check | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=521971 ---Test result--- Test Summary: CheckPatch PASS 0.37 seconds GitLint PASS 0.14 seconds Prep - Setup ELL PASS 48.34 seconds Build - Prep PASS 0.13 seconds Build - Configure PASS 8.49 seconds Build - Make PASS 224.26 seconds Make Check PASS 9.09 seconds Make Distcheck PASS 255.44 seconds Build w/ext ELL - Configure PASS 8.56 seconds Build w/ext ELL - Make PASS 201.55 seconds Details ############################## Test: CheckPatch - PASS Desc: Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS Desc: Run gitlint with rule in .gitlint ############################## Test: Prep - Setup ELL - PASS Desc: Clone, build, and install ELL ############################## Test: Build - Prep - PASS Desc: Prepare environment for build ############################## Test: Build - Configure - PASS Desc: Configure the BlueZ source tree ############################## Test: Build - Make - PASS Desc: Build the BlueZ source tree ############################## Test: Make Check - PASS Desc: Run 'make check' ############################## Test: Make Distcheck - PASS Desc: Run distcheck to check the distribution ############################## Test: Build w/ext ELL - Configure - PASS Desc: Configure BlueZ source with '--enable-external-ell' configuration ############################## Test: Build w/ext ELL - Make - PASS Desc: Build BlueZ source with '--enable-external-ell' configuration --- Regards, Linux Bluetooth
Hi Howard, On Tue, Jul 27, 2021 at 4:53 AM Howard Chung <howardchung@google.com> wrote: > > From: Yun-Hao Chung <howardchung@chromium.org> > > If foreach_data->start < svc_start < foreach_data->end < svc_end, > foreach_in_range runs foreach_service_in_range to this service. > > This patch fix the above bug. > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > --- > > src/shared/gatt-db.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index 8bff4d37aaa2..38d93f273a9e 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data) > > if (!foreach_data->attr) { > if (svc_start < foreach_data->start || > - svc_start > foreach_data->end) > + svc_end > foreach_data->end) > return; Actually if I recall this check is correct, we want to iterate to every service within the range of start to end, if the range is lets say 1-9 and the service is 1-14 is still within the range since it matches the range of 1-9, or perhaps you want to explain why that would be a problem since that is not explained in the patch description, if we do switch to strict matching it is probably worth documenting that we would only match if the entire range of services is within the range, anyway I would still keep svc_start > foreach_data->end since that would stop iterating early instead of continuing to all the list. > return foreach_service_in_range(data, user_data); > } > -- > 2.32.0.432.gabb21c7263-goog >
I see. To be honest, I didn't encounter any issues with this function. I was trying to solve another problem and found this might be a typo. Thanks for the explanation. On Wed, Jul 28, 2021 at 6:18 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Howard, > > On Tue, Jul 27, 2021 at 4:53 AM Howard Chung <howardchung@google.com> wrote: > > > > From: Yun-Hao Chung <howardchung@chromium.org> > > > > If foreach_data->start < svc_start < foreach_data->end < svc_end, > > foreach_in_range runs foreach_service_in_range to this service. > > > > This patch fix the above bug. > > > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > > --- > > > > src/shared/gatt-db.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > index 8bff4d37aaa2..38d93f273a9e 100644 > > --- a/src/shared/gatt-db.c > > +++ b/src/shared/gatt-db.c > > @@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data) > > > > if (!foreach_data->attr) { > > if (svc_start < foreach_data->start || > > - svc_start > foreach_data->end) > > + svc_end > foreach_data->end) > > return; > > Actually if I recall this check is correct, we want to iterate to > every service within the range of start to end, if the range is lets > say 1-9 and the service is 1-14 is still within the range since it > matches the range of 1-9, or perhaps you want to explain why that > would be a problem since that is not explained in the patch > description, if we do switch to strict matching it is probably worth > documenting that we would only match if the entire range of services > is within the range, anyway I would still keep svc_start > > foreach_data->end since that would stop iterating early instead of > continuing to all the list. > > > return foreach_service_in_range(data, user_data); > > } > > -- > > 2.32.0.432.gabb21c7263-goog > > > > > -- > Luiz Augusto von Dentz
> > > if (svc_start < foreach_data->start || > > > svc_start > foreach_data->end) > > > return; After discussing with Archie, if we understand you correctly, we think this early return can be removed. Let's say the searched range is 5-9 and the service is 1-14, then we should consider the service is in range. If we want to keep the early return, svc_start > foreach_data->end is already checked a few lines before, so this check is redundant.
Hi Howard, On Tue, Jul 27, 2021 at 7:51 PM Yun-hao Chung <howardchung@google.com> wrote: > > > > > if (svc_start < foreach_data->start || > > > > svc_start > foreach_data->end) > > > > return; > After discussing with Archie, if we understand you correctly, we think > this early return can be removed. > Let's say the searched range is 5-9 and the service is 1-14, then we > should consider the service is in range. > If we want to keep the early return, svc_start > foreach_data->end is > already checked a few lines before, so this check is redundant. Yep, that seems we have already had the same check so Im fine dropping the check.
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index 8bff4d37aaa2..38d93f273a9e 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -1349,7 +1349,7 @@ static void foreach_in_range(void *data, void *user_data) if (!foreach_data->attr) { if (svc_start < foreach_data->start || - svc_start > foreach_data->end) + svc_end > foreach_data->end) return; return foreach_service_in_range(data, user_data); }