Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 )
Change subject: dirtyjtag: Add DirtyJTAG programmer ......................................................................
Patch Set 7: Code-Review+1
(4 comments)
Patchset:
PS7: Thanks for your update. Alas, I missed to check the resource usage/cleanup in the init function. Everything else looks good to me :)
Also something else I kept forgetting to mention: We have some additional, annoying programmer lists in `test_build.sh`. You should add yours there too.
File dirtyjtag_spi.c:
https://review.coreboot.org/c/flashrom/+/67878/comment/321761d6_c989c674 PS7, Line 154: 32 Just curious, is there anything in the last two bytes?
https://review.coreboot.org/c/flashrom/+/67878/comment/d45918d2_18f25575 PS7, Line 212: NULL Should use the `djtag_data->libusb_ctx`. Many more below, too.
https://review.coreboot.org/c/flashrom/+/67878/comment/be295205_640c7791 PS7, Line 297: return -1; Sorry, I always forget to check such cleanup paths ._.
There are some steps missing (I guess it should do about the same as the shutdown function). However, not everything should be called on every error. For instance, calling libusb_close() before `handle` is set might be bad. Hence we'll need more labels.
Also, all early returns should go through a cleanup path. Except the first maybe, because there no resources have been allocated yet.
-
Nico Huber (Code Review)