I had some time to hack on this some more and test using a Chromebook using --fmap-file (linear search), --fmap with the fmap offset at a weird location (try bsearch and fall back to linear search), and --fmap with the fmap at a bsearchable offset.

I'm not sure what to do about the man page... The line break seems weird.

View Change

17 comments:

• File cli_classic.c:

  • Patch Set #22, Line 619: out

    out_shutdown

    Done

  • Patch Set #22, Line 642: && (flashrom_

    I stared at this for a moment, now I'm sure the `!fmapfile` is […]

    OK. I think it helps with readability since it flows nicely with the above `fmap && fmapfile`, but I don't have a strong preference.

• File flashrom.8.tmpl:

  • Patch Set #22, Line 52: \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) \

    This line is unnecessarily long now. Please break before the […]

    Yeah, it's not the longest line but it is quite long. I don't understand why you want to break before the `-n` though. The spacing doesn't make much sense to me, is there a standard or some general guidelines? I just took a guess.

  • Patch Set #22, Line 202: hrom suppo

    par*ti*tioning

    Done

  • Patch Set #22, Line 217: hrom suppo

    partitioning

    Done

  • Patch Set #22, Line 219: used to generate the layout.

    leftover

    oops, done.

• File fmap.h:

  • Patch Set #22, Line 3: -present

    I know it has been discussed already. But this is incredibly […]

    Nah, I kinda like it actually. It just says explicitly what we already assume, and prevents issues with people updating copyright notices needlessly to match the present year whenever they touch a file.

    In any case our non-lawyer opinions are irrelevant, so no point in arguing.

  • Patch Set #22, Line 45: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */

    Didn't check earlier versions, but now this is unused? shouldn't […]

    Sure.

    FWIW, the only difference IIRC between 1.0 and 1.1 was that 1.1 required alignment to a 64-byte boundary so that bsearch wouldn't be too expensive on the hardware we had available at the time. We fallback to doing brute-force linear search on all bytes anyway so it doesn't matter too much, it was intended more as a guideline for implementation.

  • Patch Set #22, Line 67: int fmap_read_from_buffer(struct fmap **fmap_out, const uint8_t *buf, size_t len);

    nit, could be static now

    Done

• File fmap.c:

  • Patch Set #22, Line 1:

    nit, missing initial line break

    Done

  • Patch Set #22, Line 153:

    	int ret = -1;
    	uint8_t *b
    

    Redundant, probably leftover?

    Done

  • Patch Set #22, Line 203: }

    leaking `fmap`

    moved above malloc

  • Patch Set #22, Line 260:

    		if (fmap_found)
    			break;
    	}
    
    	if (!fmap_found)
    		goto _free_ret;
    
    	fmap_len = fmap_size(fmap);
    	struct fmap *tmp = fmap;
    	fmap = realloc(fmap, fmap_len);
    	if (!fmap) {
    		msg_gerr("Failed to realloc.\n");
    		free(tmp);
    		goto _free_ret;
    	}
    
    	if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap),
    				offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) {
    		msg_cerr("Cannot read %zu bytes at offset %06zx\n",
    				fmap_len - sizeof(*fmap), offset + sizeof(*fmap));
    		/* Treat read failure to be fatal since this
    		 * should be a valid, usable fmap. */
    		goto _free_ret;
    	}
    
    

    This path is flawed a little. At least it's missing an unconditional […]

    The break seems to have been lost during refactoring :-/ Sigh.

    I like the idea of doing more stuff outside of the inner loop. I don't think we should ever hit the realloc() a second time, and it should be fine to treat those error paths as fatal.

    I suppose one could make an argument that if we see a valid fmap header but can't read the whole thing then we should continue trying to find another fmap. But I think in practice that is indicative of a hardware failure or error in the way the firmware ROM is laid out and it's useful to treat it as fatal. Maybe we should return a different code to the caller to indicate this and avoid doing the linear search.

  • Patch Set #22, Line 292: ret = 0;

    This would still be set after a read failure, for instance. So it's […]

    agreed

  • Patch Set #22, Line 321: an fma

    How do you pronounce it? Is it `*a* fmap` or `*an* eff-map`?

    I say "an" in my head. Usually 'a' is used before a noun beginning with a consonant, but that sounds awkward to me in this case so I've updated the comment.

• File libflashrom.c:

  • Patch Set #22, Line 433: struct flashctx *const flashctx, off_t offset, size_t len)

    Ugly, but as we were too lazy for proper deserialization again, […]

    done

  • Patch Set #22, Line 468: * 3 if fmap parsing isn't implemented for the host,

    Same guard here.

    Done

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