There are still some issues when it comes to the updated behaviour due
to that http://crbug.com/263495

• Manpage needs update.
• Separation of this and the next commit makes no sense because the introduced if/else relies on the file arguments to -w/-v being optional.
• I think we need a check for the case that both files to -w/-v and to -i are given (it seems to me the latter would be silently ignored, didn't test though).

View Change

10 comments:

• File flashrom.8.tmpl:

  • Patch Set #7, Line 227: write

    spurious

  • Patch Set #7, Line 229:

    For write operations, files specified using
    .B "\-i"
    take precedence over content from the argument to
    .B "\-w."
    

    That's no more true. -i and -w filenames are mutually exclusive, right?

  • Patch Set #7, Line 252: .B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -w rom.bin

    Hmmm, just realized that this doesn't work. The -i files are just ignored
    when -w gets a file passed.

    I'd be fine if you document the final behaviour here. But maybe it's yet
    easier to squash the two commits...

• File flashrom.c:

  • Patch Set #7, Line 1380:

    		if (!entry->included || !entry->file)
    			continue;
    

    Optionally:

        if (!entry->included)
                continue;
        if (!entry->file) {
                msg_gerr("Error: No image file given for region \"%s\".\n", entry->name);
                return 1;
        }

    I know this is already covered in the next commit for the CLI, but I
    would feel much better if the code here looks consistent.

  • Patch Set #7, Line 1383: buf

    + entry->start ?

  • Patch Set #7, Line 1464: const char *filename)

    Should fit a single line.

  • Patch Set #7, Line 2647: * processed and merged into newcontents since -i files take priority.

    This comment still makes no sense to me. I'd guess it was
    written before the if / else decision was introduced.

• File layout.c:

  • Patch Set #7, Line 239: 0

    i + 1 ?

  • Patch Set #7, Line 244: continue;

    Not needed if we start with `j > i`.

  • Patch Set #7, Line 272:

    	for (i = 0; i < layout.num_entries; i++) {
    		free(layout.entries[i].file);
    		layout.entries[i].file = NULL;
    		layout.entries[i].included = 0;
    	}
    	layout.num_entries = 0;
    

    This should actually be done in flashrom_layout_release() (before
    the `if (layout == get_global_layout())` check).

    It's a bit ugly atm, because nobody took the time yet, to untangle
    the CLI parts and the static layout struct here. Hmmm, now it
    doesn't look like much work: read_romlayout() would allocate a new
    layout, normalize_romentries() should work on the layout linked to
    the `flashctx`. Everything else doesn't seem to use the static
    struct.

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