Hi Daniel, On 20.05.22 17:51, Daniel Campello wrote: > I decided to take a look at https://ticket.coreboot.org/issues/372 and I > would like to get a better understanding of what is being requested on the > issue tracker. I took a look at the submitted patch (comments and code > diff) and everything seems in order to me.
thanks for bringing this to a broader attention. I did have a look again and at least my latest suspicion is not true. The last patch set removed some #if compatibility for libpayload (and then was merged pretty quick- ly), but that seems alright, it probably just was a remnant of the too many rebases. Speaking of rebases, this was my second concern: Patch set 16 wasn't a clean rebase. It undid a lot of already discussed changes. And, alas, you based your final work on top of that. Now when people gave +2 and merged it, I guess they didn't knew that the code doesn't reflect what was discussed on Gerrit. I just had a very quick look. The only things I noticed: Some OOM conditions are not caught in register_include_args(). And the man- page talks about `--include` which is actually still implemented as `--image`. Probably another case of people lost track on what version of the code the patch based. As it was a very big patch and a rather messy review, I think it would be best to have somebody have a thorough look through all the changes. Preferably somebody who didn't work on the patch because it's not easy to spot one's own errors. Nico PS. I also brought up during the last dev meeting that the API would look very differently if we would implement it today (e.g. to allow better separation of the CLI and internals). However, I didn't realize that the patch already works on its own (I had the wrong impression that it requires the unmerged CLI change, but it actually works already if one provides a dummy file to -r/-v/-w; just tested). _______________________________________________ flashrom mailing list -- [email protected] To unsubscribe send an email to [email protected]
