Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine.
12 comments:
Patchset:
Thank you so much for combining all the work in one patch!
File erasure_layout.c:
I'm a little hesitant to, because `chipoff_t` implies inclusive endpoints. […]
I forgot that `chipoff_t` has its own comment. Now I agree with you.
Patch Set #5, Line 369: len = min(region_end, region.end - 1) - addr + 1;
I added a new parent change that fixes get_flash_region's incorrect use of chipoff_t (now the upper […]
This is great, thank you!
File tests/erase_func_algo.c:
To be consistent with your previous patch, and also I think tests are partially a documentation (a special kind of it), this needs to be 15, right?
Patch Set #9, Line 951: get_erase_protected_region_algo_tests
I think these ones can be also run through write operation, and verifying the memory state after write. The change in flashrom.c affects write as well.
But this can be in a separate patch.
Patch Set #9, Line 953: const size_t num_unparameterized = 1;
Do you think this is needed? It's the same name and the same function. If you change it to 2, it will run the same test twice. Maybe remove this and allocate one test?
Patch Set #9, Line 973: .name = "erase failure for unskipped unwritable regions",
I had the trouble with test names, because I wanted an index (a test case #) in the name. This one is just one, so a name without an index is fine.
I looked into the test `test_erase_fails_for_unwritable_region` I think it's only flag FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS that prevents in from being in the list? setup and teardown seem innocent, won't hurt to have them here. So it's not being unparameterized , but FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS=false is what makes this test different?
Anyway, this can be changed later.
This is `END_PROTECTED_REGION + 1`, just for perfection
(with my other comment about END_PROTECTED_REGION)
<=
if you agree that it should be inclusive
also here <=
File tests/tests.h:
Patch Set #9, Line 113: void erase_unwritable_regions_skipflag_off_test_success(void **state);
this also can be removed from this patch
Patch Set #9, Line 114: erase_unwritable_regions_skipflag_on_test_success
I am wondering maybe this is also not needed. Because the function which is called in tests.c is `get_erase_func_algo_tests` but not this one.
I added this line, but then also tests ended up having a special invocation and I think it's not needed anymore.
To view, visit change 82393. To unsubscribe, or for help writing mail filters, visit settings.