Inaky Perez-Gonzalez has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )
Change subject: dediprog: add serial argument ......................................................................
Patch Set 3:
(6 comments)
Updated hte patch set; I believe all feedback is addressed and by allowing id and serial_number together we can address the case when two different dongles have the same USB serial number. If they also have the same ID number, not much that can be done unless we'd want to add filtering by USB path, which would be a different task.
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c File dediprog.c:
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1008 PS2, Line 1008: * @index index of the USB device : * @serial_number serial number of the USB device (id is ignored then) : * @return 0 for success, -1 for error, -2 for busy device
Please re-align.
Ack
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1065 PS2, Line 1065: *serial_number;
Line break seems unnecessary.
Gets a couple chars over 80 column limit; I'll paste it up.
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1099 PS2, Line 1099: serial_number = extract_programmer_param("serial");
Leaking `serial_number` and missing check if conflicting options (id, device) […]
I do agree on the leakage but...does it matter? Meaning if this is a one shot execution tool and once done all resources are freed by the OS, it's done--or am I wrong and there is a library usage of this I am not aware of where resource allocation would accumulate into a leakage?
Re id vs serial, this could be used to work around when identical USB serials are in the system (comment somewhere else in the thread), so this check could be an AND (id == SOMETHING and serial_number == SOMETHINGELSE). I'll rework a wee bit.
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1192 PS2, Line 1192: if (serial_number) { : if (dediprog_open(0, serial_number)) { : return 1; : } : found_id = dediprog_read_id();
Can be handled more elegantly in the else path below.
oh, collapse both--took me three times to figure out what you meant--very good point, thx.
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1232 PS2, Line 1232: NULL
Could just pass `serial_number` here. Either one of the variables would be 0.
Done
https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1236 PS2, Line 1236: }
From here, we won't need `serial_number` anymore.
Done