Attention is currently required from: Nico Huber, Subrata Banik, Edward O'Callaghan. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62213 )
Change subject: flashrom.c: Implement file-based locking semantics ......................................................................
Patch Set 2:
(3 comments)
File big_lock.c:
https://review.coreboot.org/c/flashrom/+/62213/comment/1bda94c6_23a8a0ff PS1, Line 39: return acquire_lock(&big_lock, timeout_secs * 1000);
Seems unnecessary complexity to make it conditional since we would always want it to be the case tha […]
I'm concerned the file-based locking may not work on all systems, for example Android or Windows, or any other systems that previously supports flashrom without problems.
File big_lock.c:
https://review.coreboot.org/c/flashrom/+/62213/comment/f8ca8977_704e1087 PS2, Line 37: firmware_utility_lock should this be flashrom_lock ?
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/62213/comment/e2d2e50f_5c714c0f PS1, Line 142: programmer_entry
afaik, this is issue is specifically related to futility tests not yet running under platform2_wrapp […]
Not just futility - this is more like "can you run flashrom without getting root permission (required by lock)". I think people will want to simulate a flashrom call using dummy programmer (to create an image file) without adding 'sudo'.
But on the other hand, it's also probably not a bad idea if we always require 'sudo' - even for unit tests. I'm fine if you want to keep the biglock always there.
One more thing to check: we won't use two programmers simultaneously right? Because currently the lock is a simple 1/0 state. We may need to consider a real singleton (e.g., add ref counter) if some programs calling libflashrom will try to get two programmers , for example read from one and write to the other. (flashrom doesn't do this but we don't know what libflashrom users will do)
-
Hung-Te Lin (Code Review)