looks much better, thank you. I like the movement to fmap.c

View Change

15 comments:

• File cli_classic.c:

  • Patch Set #20, Line 107: "r"

    "rb" for windows? not sure

• File fmap.h:

  • Patch Set #20, Line 69: const

    `const` on the parameter itself is meaningless in the contract

• File fmap.h:

  • Patch Set #19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free

    It's dual-licensed, as the last paragraph states. […]

    That's why I though, why mention GPLv2 explicitly? Also wondering
    if we miss something ;)

    Ok checked, it's the standard 3-clause BSD text. Generally GPL
    compatible... weird.

• File fmap.c:

  • Patch Set #20, Line 44: int fmap_size(const struct fmap *fmap)

    could be static now

  • Patch Set #20, Line 47: return -1;

    All callers seem to know that it's non-NULL but ignore possible
    negative return values. Maybe just remove the check and return a
    size_t?

  • Patch Set #20, Line 59: flash

    What you actually check here is not the overall flashsize itself but the size of this FMAP instance […]

    Are there nested FMAPs? I've only seen the default ones in coreboot
    and they seem to cover the complete flash.

  • Patch Set #20, Line 151:

    	off_t fmap_offset = fmap_lsearch(buf + rom_offset, len);
    	if (fmap_offset < 0) {
    		msg_gdbg("Failed to find fmap using linear search.\n");
    		goto _free_ret;
    	}
    
    	int fmap_len = fmap_size((struct fmap *)&buf[fmap_offset]);
    	*fmap_out = malloc(fmap_len);
    	if (*fmap_out == NULL) {
    		msg_gerr("Out of memory.\n");
    		goto _free_ret;
    	}
    
    	memcpy(*fmap_out, buf + fmap_offset, fmap_len);
    

    Beside the error message, this is probably the same as calling
    fmap_read_from_buffer().

  • Patch Set #20, Line 179:

    	unsigned int chip_size = flashctx->chip->total_size * 1024;
    	int sig_len = strlen(FMAP_SIGNATURE);
    

    make both const?

  • Patch Set #20, Line 182:

    if (!flashctx || !flashctx->chip)
    		return 1;
    

    Would you like to add this check to fmap_lsearch_rom() as well?

    Or just move it up into fmap_read_from_rom()?

  • Patch Set #20, Line 256:

    struct fmap *tmp = fmap;
    			fmap = realloc(fmap, fmap_len);
    			if (!fmap) {
    				msg_gerr("Failed to realloc.\n");
    				free(tmp);
    				goto _free_ret;
    			}
    

    realloc is allowed to move the re-allocated memory buffer to a completely new address[1]. […]

    realloc() frees the original memory if moved. So we only have
    to take care of the original pointer (tmp) in the error case,
    what we do.

• File libflashrom.h:

  • Patch Set #20, Line 67: const

    the later const is not meaningful here

• File libflashrom.c:

  • Patch Set #20, Line 396: if (l->num_entries == MAX_ROMLAYOUT) {

    we could also check in advance `if (fmap->nareas > MAX_ROMLAYOUT)`
    but this works too

  • Patch Set #20, Line 405: sizeof(l->entries[i].name)

    FMAP_STRLEN is shorter, use that or MIN() of both?
    Also an explicit 0-termination would be good (I know
    there should be one in the FMAP, but we didn't check
    the entries, did we?)

  • Patch Set #20, Line 477: ret = 1;

    redundant

  • Patch Set #20, Line 484: ret = 1;

    redundant

To view, visit change 23203. To unsubscribe, or for help writing mail filters, visit settings.