Request for clarification on "per-region file arguments" issue

[Daniel Campello]

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

[Nico Huber]

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).

[Anastasia Klimchuk]

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?