Skip to content

BUG: fix 54-live-binary_tree to use binary tree#371

Closed
kolyshkin wants to merge 1 commit intoseccomp:mainfrom
kolyshkin:tests-54-tfix
Closed

BUG: fix 54-live-binary_tree to use binary tree#371
kolyshkin wants to merge 1 commit intoseccomp:mainfrom
kolyshkin:tests-54-tfix

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

Apparently, an early implementation of the binary tree optimization
used to enable the feature when the number of rules added was > 16.

The code was later changed to add and use SCMP_FLTATR_CTL_OPTIMIZE,
but the 54-live-binary_tree test case was left as is. So, despite
its name, it is not testing the binary tree.

Fix this, and remove the comment that referred to the old
implementation.

Fixes: 38f04da (PR #152)
Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Seems that CI is screwed because of an inconsistent state of Azure's Ubuntu apt mirror, e.g.

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/g/glibc/libc6-dbg_2.31-0ubuntu9.2_amd64.deb 404 Not Found [IP: 40.81.13.82 80]

@drakenclimber
Copy link
Copy Markdown
Member

Seems that CI is screwed because of an inconsistent state of Azure's Ubuntu apt mirror, e.g.

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/g/glibc/libc6-dbg_2.31-0ubuntu9.2_amd64.deb 404 Not Found [IP: 40.81.13.82 80]

I re-kicked off the CI. Fingers crossed.

@drakenclimber
Copy link
Copy Markdown
Member

Looks good to me. Thanks @kolyshkin.

Reviewed-by: Tom Hromatka <tom.hromatka@oracle.com>

@drakenclimber drakenclimber requested a review from pcmoore March 15, 2022 20:21
@drakenclimber drakenclimber added this to the v2.5.4 milestone Mar 15, 2022
@pcmoore
Copy link
Copy Markdown
Member

pcmoore commented Mar 15, 2022

I just kicked the CI too as the last run was still failing with the same problem.

@pcmoore
Copy link
Copy Markdown
Member

pcmoore commented Mar 15, 2022

Oh, I found the CI problem, just a minute ...

@pcmoore pcmoore changed the title tests: fix 54-live-binary_tree to use binary tree BUG: fix 54-live-binary_tree to use binary tree Mar 15, 2022
@pcmoore
Copy link
Copy Markdown
Member

pcmoore commented Mar 15, 2022

Okay, our CI should now be fixed (we we're missing an apt-get update to ensure our apt repo metadata wasn't stale) in 7a28dfa or later. @kolyshkin would you mind rebasing your PR against the new libseccomp/main branch? If that's a problem let me know and I'll just review/test it against the updated main.

@drakenclimber since this was a trivial change and we needed it to get the CI working again I just went ahead and merged the fix, if you have any concerns about it let me know and we can patch over it or revert it if something is really wrong.

@drakenclimber
Copy link
Copy Markdown
Member

@drakenclimber since this was a trivial change and we needed it to get the CI working again I just went ahead and merged the fix, if you have any concerns about it let me know and we can patch over it or revert it if something is really wrong.

Totally the right thing to do. No objections here.

Apparently, an early implementation of the binary tree optimization
used to enable the feature when the number of rules added was > 16.

The code was later changed to add and use SCMP_FLTATR_CTL_OPTIMIZE,
but the 54-live-binary_tree test case was left as is. So, despite
its name, it is not testing the binary tree.

Fix this, and remove the comment that referred to the old
implementation.

Fixes: 38f04da
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

Okay, our CI should now be fixed (we we're missing an apt-get update to ensure our apt repo metadata wasn't stale)

Ah! I briefly suspected that yesterday but neglected to check :-\

would you mind rebasing your PR against the new libseccomp/main branch

Done

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 89.601% when pulling 9779813 on kolyshkin:tests-54-tfix into 7a28dfa on seccomp:main.

@pcmoore
Copy link
Copy Markdown
Member

pcmoore commented Mar 16, 2022

Merged via 5731b3c and 861e2dc, thanks!

@pcmoore pcmoore closed this Mar 16, 2022
@kolyshkin kolyshkin deleted the tests-54-tfix branch March 17, 2022 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants