Patch Set 4:

(17 comments)

Looks good over all. The only real issues that have to be fixed is
a leaked `FILE *` and internal URLs, IIRC. Mostly everything else
is in the "we can do better" category ;)

Thanks for the review. I did a sanity check with a Chromebook that uses MTD, but hit a snag when converting write() to fwrite(). I'll take another look tomorrow and update the man page as well.

View Change

17 comments:

• File flashrom.c:

  • Patch Set #4, Line 352:

    Remove the ".*" at the end since MTD files don't use minor numbers (AFAIK).

• File internal.c:

  • Patch Set #4, Line 241: msg_perr("Processor detection/init failed.\n"

    A little nicer would be an inline stub in the header file in […]

    Done. I also moved the call to the inline above the call to pci_init_common() since MTD will obviate the need for it if successful.

• File linux_mtd.c:

  • Patch Set #4, Line 59:

    Using low-level read() might be ok here together with sysfs. […]

    Done

  • Patch Set #4, Line 94: read_sysfs_string(sysfs_pat

    Nit, same as `*endptr`?

    Done... I think? LMK if I misunderstood your comment.

  • Patch Set #4, Line 131: read_sysfs_int(sys

    How portable is this?

    Should be fairly portable and is supported by all modern versions of GCC and clang AFAIK. However now that you mention it I don't see mingw support so I went ahead and replaced it with a generic popcnt implementation.

  • Patch Set #4, Line 152:

    This only runs once on init, right? msg_pdbg() would be more […]

    Done

  • Patch Set #4, Line 167: }

    Nit, line break seems unnecessary.

    Done

  • Patch Set #4, Line 187:

    
    static int linux_mtd_re
    

    I doubt, my browser would find much there.

    Removed

  • Patch Set #4, Line 193:

    Again, fread() is generally preferred. read() doesn't retry if […]

    Done

  • Patch Set #4, Line 224:

    sers[0].eraseblocks[0].size;
    	unsigned int i;
    

    ...

    Done

  • Patch Set #4, Line 231: perr(

    Same as for read().

    I encountered a "Bad file descriptor" error when attempting to convert this to fwrite(), but will look into it more tomorrow. Hopefully it was just a silly error.

  • Patch Set #4, Line 256:

    Bail out like above?

    Done

  • Patch Set #4, Line 294: .read = linux_mtd_read,

    Never closed? I would move it on top, initialize to `NULL` and […]

    Yeah, may as well use read_sysfs_string(). Done.

  • Patch Set #4, Line 320: char dev_path[32];

    `goto linux_mtd_setup_exit;`

    Done.

  • Patch Set #4, Line 326: etup_exit;

    Nit, why cast?

    Hmmm, I thought there were compiler warnings earlier on, but I don't seem to be getting them now. I'll remove the cast for now.

  • Patch Set #4, Line 361: n 0;

    `*endptr` would also cover trailing data.

    Done.

  • Patch Set #4, Line 364: int linux_mtd_init(void)

    Nits, missing full-stop, output should be broken before 80 chars.

    Done

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