Attention is currently required from: Daniel Campello, Angel Pons, Anastasia Klimchuk.
Patch set 4:Code-Review +2
1 comment:
Patchset:
> 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.
To view, visit change 59921. To unsubscribe, or for help writing mail filters, visit settings.