On Sat, 18 Aug 2012 22:33:30 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 15.08.2012 04:29 schrieb Stefan Tauner:
On Wed, 15 Aug 2012 02:03:29 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 13.08.2012 02:51 schrieb Stefan Tauner:
-int show_id(uint8_t *bios, int size) +/* Tries to find coreboot IDs in the supplied image and compares them to the current IDs.
- Returns...
- -1 if IDs in the image do not match the IDs embedded in the current firmware,
0 if the IDs could not be found in the image or if they match correctly.- */
+int cb_check_image(uint8_t *image, int size)
cb_check_image_matches_board would be a better name IMHO.
it reflects the current behavior better, yes. but OTOH it seems to have started as a function that was just showing the ID and the name was not changed until now, although the behavior changed a lot. i dont like to name functions too precisely because it can easily become wrong :) also, it is much longer and now there is a comment explaining exactly what it does in case someone does not know.
I envision lots of similar functions in the future, e.g. for checking the PCI ID embedded in an option ROM file against the PCI ID of the target device. check_image_matches_optionrom_pcidevice() and check_image_matches_board_awardbios() and check_image_matches_board_coreboot()... so the name I suggested isn't optimal either. If we use check_image_matches_ as prefix, we'd have a consistent prefix for any such future functions. Your choice.
renaming should be done in the patch that adds the first other function then (i am not sure that those function will (all) be part of flashrom itself).
if ((*walk) == 0 || ((*walk) & 0x3ff) != 0) {
/* We might have an NVIDIA chipset BIOS which stores the ID information somewhere else. */walk = (unsigned int *)(bios + size - 0x80);
/* We might have an NVIDIA chipset which stores the ID information somewhere else. */Actually, "BIOS" wasn't that wrong... let me rephrase the comment so that others may have a chance to understand the reason:
/* Some NVIDIA chipsets store chipset soft straps (IIRC Hypertransport init info etc.) in flash at exactly the location where coreboot image size, coreboot vendor name pointer and coreboot board name pointer are usually stored. In this case coreboot uses an alternate location for the coreboot image data. */
i dont see how this makes BIOS less wrong, but thank you very much for the clarification!
You could leave the comment here alone, and I could send a separate patch with the offset fixups I suggested and with the new comment I suggested. Your choice.
since i have already added you comment instead of the previous, changed comment, and because it does make sense even without the offset change ill commit it now.
@@ -242,22 +206,15 @@ static void find_mainboard(struct lb_record *ptr, unsigned long addr)
find_mainboard is a misnomer... it should be get_mainboard_from_cb_record or something like that, and the addr parameter was never used since the code was first committed. Followup patch, no need to complicate this one.
true, but OTOH it is just a static method...
My concern was code readability, not public interfaces. You're right that this code fortunately isn't called from outside this file. To be honest, this file is probably (except layout.c) the least modified file since flashrom development started.
oh i figured that out... it is a pity that code dragons just dont die from old age. :)
changes since the last iteration: - trivial manpage change - renaming unsafe_board_handler to is_board_enable_safe. this changes the truth! ;) so there are a few related changes including a small fix/sanitation of board_handle_phase
whee almost done