On 3/4/26 6:45 PM, Eric Biggers wrote:
On Wed, Mar 04, 2026 at 10:34:37AM +0100, Milan Broz wrote:
On 3/4/26 10:00 AM, Eric Biggers wrote:
On Wed, Mar 04, 2026 at 09:25:02AM +0100, Milan Broz wrote:
Okay, it sounds like the verity-compat-test script isn't open for
contributions then. Would have been nice to know earlier, but thanks
for letting me know now. I'll plan to start some new tests for
kselftests.
That's not what I said, just I would prefer adding new test without
the need to completely reformatting the test.
Adding new test cases to that script naturally requires understanding a
lot of the existing code so the new cases can be properly integrated.
For example new test cases logically should use the existing helper
functions, common global variables, and failure reporting mechanism; and
they shouldn't duplicate or interfere with the existing test cases. And
in some cases a new test case may be much more easily handled as an
extension or fix to an existing one; for example the easiest way to test
fec_roots != 2 probably would be to just fix check_fec() to honor its
existing fec_roots parameter as seems to have been intended.
But with the existing script not following modern shell-scripting
practices like naming parameters, using local variables, and passing
'shellcheck', I found that to be a challenge for understanding the
existing code and adding new code. Especially when these practices are
causing bugs, like the ones my merge request would have fixed.
So that's why I thought a broader cleanup of this test script would be
really useful first. And I was glad to do that, which I did. None of
this was meant of a criticism of you; I'm just trying to help.
But it's clear I wasted my time and should focus my efforts on the
kernel, kselftests, and other userspace projects. So I'll continue to
do that and leave cryptsetup alone. Sorry for trying to contribute.
Hi Eric,
I actually agree with you on almost everything above.
But this is not the only test script we have.
If we decide to do cleanup, things like ">/dev/null 2>&1" to "&>/dev/null",
or wrap long lines (as in your series), I would like to do this across
the entire testsuite. And actually, I can do that myself; I just need to plan
it.
These changes are pure cosmetics.
Then there are some real cleanups, like local variable naming,
that make sense to apply now. If you submit these with real test additions,
I already merged them to the main branch.
This is the diff I am talking about
https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/890/diffs
It's common for maintainers to have requirements for code changes.
I think we both know that.
I would like to see that both the kernel and the cryptsetup project have good
code coverage for dm-verity. Kernel has none currently.
As we already have a testsuite, it would be nice and probably much easier to add
new corner cases to test your recent dm-verity improvements.
But please also try to respect what I said above.
Thank you,
Milan
