Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 )
Change subject: dummyflasher.c: Fix data leak in params processing error paths ......................................................................
Patch Set 5:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54748/comment/7eb8ca94_0de7188e PS4, Line 20:
I haven't seen this tag before, is it used often? I saw people mention in commit message "follow-up […]
Heh, I actually prefer text over the Fixes: tag ;) The tag is just what other people use... probably more common in coreboot.
Regarding CB:, it's a layer of indirection. Before a commit is submitted to master, we can only reference the change, because the commit hash is not final. Pro: Gerrit links it conveniently. Con: Native Git tools know nothing about it and one would have to search for the number manually.
For already merged commits, I prefer to use the commit hash and summary. IIRC, if it's prefixed with `commit`, Gerrit also links it. Eventually, this Gerrit instance may not run anymore, but the Git history prevails.
NB. I'm not a fan of forward references, jumping back and forth can make the review harder. There are exceptions ofc.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/150dc8e6_022d2882 PS1, Line 26: /* Remove the #define below if you don't want SPI flash chip emulation. */ : #define EMULATE_SPI_CHIP 1
Just to confirm, you mean, let's assume (EMULATE_CHIP && EMULATE_SPI_CHIP) always true and drop the code outside the #if? So that dummy will always be dummy_spi? But then maybe have another one dummy_par? Maybe someone needs those different values.
`dummy` is already both `dummy_spi` and `dummy_par` at once. Both masters are registered if the commandline says they should be supported. EMULATE*_ CHIP is only about emulating a flash chip connected to the bus. I don't see why we couldn't have one chip on each bus at once.
Not sure what you mean with "drop the code outside the #if". All the code is compiled in currently. There is no #else that would be dropped.
I don't know what was the initial meaning of !EMULATE_CHIP use case. What dummy is doing if it is not emulating any chip? I see it can emulate spi or par, I can understand, but not emulating anything?
It's emulating buses, that's already enough to test flash probing for instance. But the behavior should be the same as with `EMULATE_CHIP == 1` and no chip given on the commandline, I guess. So I really don't see a reason for the #if mess.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/c18345a1_f5a4b487 PS4, Line 994: dummy_buses_supported
I was thinking about it, and my guess was, because it is only needed for init time, and not needed l […]
It is not only used for init. But there lies the answer: it's used get the data pointer in get_data_from_context(); so it can't be part of it. However, it seems unnecessary, `flash->mst` also has a `buses_supported` field? Very odd.