dfu-util pull request

Stefan Schmidt stefan at datenfreihafen.org
Sat Apr 21 23:01:52 CEST 2012


Greetings from UK! I'm here for one week now and and work and life
still needs a lot of time to settle. Thus I really appreciate you
preparing a pull request putting all the stuff in one place for me to
review and pull from. Saves a lot of my time.

On Wed, 2012-04-18 at 23:04, Tormod Volden wrote:
> I think we are close to 0.6 now :) I fixed the overwriting files issue
> in the stdio porting, and finished off other small things on my to-do
> list. There are many patches so I am not going to spam the list with
> them. Please pull or review it from
> http://gitorious.org/unofficial-clones/dfuse-dfu-util/graph/master-patches

> If you see any issues or want to comment on a patch, I can also submit
> individual patches to the list for discussion, just let me know.

I just tested the complete set with all my devices and found no
regression. Good job.

Besides some smaller comments this all looks pretty nice.

> The following changes since commit 5d48b25082300d1bcc62b897b2a0d4a74cf75806:
>   Fix segfault regression in option handling (2012-02-24 23:24:55 +0100)
> are available in the git repository at:
>   git://gitorious.org/~tormod/unofficial-clones/dfuse-dfu-util.git
> master-patches
> for you to fetch changes up to 2fc823fcdc1773c000c29685e0da7ec9483ba95e:
>   Add quirk for Maple since it reports wrong DFU version (2012-04-18
> 22:38:28 +0200)
> ----------------------------------------------------------------
> Stefan Schmidt (2):
>       dfu_file: Add function to generate and add a DFU suffix to a file

Thanks for the leak fix on this one.

>       dfu-suffix: New DFU suffix manipulation tool

> Tormod Volden (21):
>       dfu_file: Verify that we could read the whole file
>       dfu_file: Return failure if CRC does not match
>       dfu-suffix: Print version before banner, and do it every time

All above are fine.

>       Port file handling to stdio streams
>       Do not overwrite existing files when uploading

Might be a good idea to squeeze the overwrite fix into the file
handling commit. Its a fix for this commit anyway. Changes the other
things incremental is fine but without this squashed we would have a
regression. Especially one that overrides a user file. :/ Sure
therefore the user would need to compile it with the file IO port
patch but without the fix. Still good practice.

The fseek() for sync read/write streams was interesting, btw.

>       dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

>       Check for availability of getpagesize()
>       Replace usleep() and sleep() by milli_sleep() macro

Is HAVE_WINDOWS_H really the preferred way to check for windows? Satz
used ENV_WINDOWS (which also looked odd to me). Could be that it
really depends on what you are using to create the executable. MinGW
vs. Windows compiler. Anyway, as you have tested this under Windows
and I also have no problems with it under Linux thats fine.

>       configure.ac: Remove unnecessary tests
>       Remove some obsolete, unused code
>       Use standard library (C99) types instead of OS types
>       Only conditionally include unistd.h
>       usb_dfu.h: Pack struct properly for Microsoft compilers
>       Various "pedantic" code compliance fixes

The usage string split into three printfs looks pretty ugly to me.

>       Const'ify some strings and add some explicit casts
>       Do not use leading underscores in #include guards
>       dfuse: Add progress indication on download

For consistency I would go with a # here instead of a dot. The
non-DfuSe part of dfu-util uses it for progress indication.

>       Detect DfuSe device correctly on big-endian architectures
>       main: Stop guessing transfer size, error out instead

I agree, its about time to rely on the functional descriptor. Lets see
if any bug reports will come for this. :)

>       main: Update copyright banner
>       Add quirk for Maple since it reports wrong DFU version

So this is without confirmation from leaflabs, right? Its fine anyway
as it can only improve the situation. :)

The squashing of the bug fix in the previous change is the only thing
that would block me from pulling this in. If you could fix this up I
would happily pull in an updated pull request.

More information about the devel mailing list