View Change

15 comments:

• File cli_classic.c:

  • Patch Set #5, Line 61: <:

    rather :<

• File flashrom.c:

  • Patch Set #5, Line 1320: const char *filename, const char *size_msg)

    Line limit is 112 chars for flashrom.

    (not my first choice, though hadn't time to look at the
    discussion, back in 2011~2012 iirc)

  • Patch Set #5, Line 1364:

    #ifdef __LIBPAYLOAD__
    	msg_gerr("Error: No file I/O support in libpayload\n");
    	return 1;
    #else
    

    Seems unnecessary, now that we use read_buf_from_file().

  • Patch Set #5, Line 1368: int ret = 0, i;

    `ret` is not used (as a variable) anymore.

  • Patch Set #5, Line 1379: }

    I'm a bit confused here. Do we allow such layouts at all? Will
    try to investigate myself...

  • Patch Set #5, Line 1387: if ((entry->start > size) || (entry->end > size)) {

    Checking `entry->end` should suffice.

  • Patch Set #5, Line 1391: }

    Again, do we allow such layouts?

  • Patch Set #5, Line 1484:

    #ifdef __LIBPAYLOAD__
    	msg_gerr("Error: No file I/O support in libpayload\n");
    	return 1;
    #else
    

    Should be kept in do_write_buf_to_file() where the actual file handling
    happens. It also shows that flashrom.c is not the right place for this
    maybe worth a follow-up.

  • Patch Set #5, Line 1502: msg_gdbg("\n"); /* avoid printing in caller's output */

    `i == 0` isn't always the first included region

  • Patch Set #5, Line 2660:

    	/*
    	 * This must be done before any files specified by -i arguments are
    	 * processed and merged into newcontents since -i files take priority.
    	 * See http://crbug.com/263495.
    	 */
    

    I don't understand it. The comment seems to assume that we call
    both after another?

    Also we should catch it in the CLI in the next patch. So we have
    a proper error message in case both a -w filename and -i :files
    are given. Same for -v.

• File layout.c:

  • Patch Set #5, Line 175: -l/--layout

    This list might get very long...

  • Patch Set #5, Line 189: /* Quotes or whitespace are not allowed in region names. */

    why test it here? why add it now?

  • Patch Set #5, Line 202: found++;

    As we bail out otherwise, `found` will always be `num_include_args`
    in the end?

  • Patch Set #5, Line 213:

    #ifdef __LIBPAYLOAD__
    			msg_gerr("Error: No file I/O support in libpayload\n");
    			return 1;
    #else
    

    libflashrom doesn't call it anyway, does it? The original guards
    only exist to make it compile (i.e. hide calls to i/o stream
    functions).

  • Patch Set #5, Line 217: file = strdup(file);

    why? is it necessary? doesn't `file` point into `argv` of main()?

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