Hello,
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.
I appreciate any assistance you can provide,
Regards, Daniel Campello
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).
Hello Daniel, Nico,
I noticed this:
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.
Looking at the patch, it seems like almost everyone already worked on it - either as developer or reviewer (which is not surprising given that it has been 3.5 years under review and has changed many owners). So there are not many people who can pass the condition "somebody who didn't work on the patch" , however I realised that I do pass! :) So I can do this, have a look and see if I spot something.
Just to be clear, unless someone objects, I am only taking item #3 from the previous email (I can see there are 3 todos in the email). Does this sound good?
-
Anastasia Klimchuk -
Daniel Campello -
Nico Huber