Attention is currently required from: Daniel Campello, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59921 )
Change subject: flashrom.c: extract operation only uses layout files ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Patchset:
PS4:
I don't understand the concern that goes away when "filename check would move from read_flash_to_f […]
I tend to agree with Daniel here. I am not sure what the concern is since read_flash_to_file() has a singular call site and CB:59291 is simply a matter of inlining read_flash_to_file() to the call site. To Daniel's point, it isn't about what the code could be doing but rather what it is actually doing.
In relation to the question of if do_read() *should* or *could* take 'filename' as type optional, well the user is write_buf_to_file() which returns 1 upon nullarity which is the bug that this patch fixes and that all other use cases of do_read() within cli_classic have 'filename' validated with, `if ((read_it | write_it | verify_it) && check_filename(filename, "image"))`. Therefore the *should* is redundant and arguably more error prone as it caused the bug in the first place since validation is happening at multiple layers in a inconsistent manner.
I however see your logic however a deeper inspection reveals that we should make the top of the call stack responsible for the semantics of whether 'filename' is optional or not and not at a 'low level'. This results in consistent expectations of the caller of who is responsible. Shame C doesn't have types for this to put into the function signatures.
I would be far more in favor of a bug fix being merged as a priority over a refactor patch that simply inlines a function which is an orthogonal problem.
-
Edward O'Callaghan (Code Review)