dfu-util pull request

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

dfu-util pull request

Tormod Volden
Hi,
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.

Cheers,
Tormod

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
      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
      Port file handling to stdio streams
      Do not overwrite existing files when uploading
      dfu-suffix: Check for availability of truncate() function
      Check for availability of getpagesize()
      Replace usleep() and sleep() by milli_sleep() macro
      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
      Const'ify some strings and add some explicit casts
      Do not use leading underscores in #include guards
      dfuse: Add progress indication on download
      Detect DfuSe device correctly on big-endian architectures
      main: Stop guessing transfer size, error out instead
      main: Update copyright banner
      Add quirk for Maple since it reports wrong DFU version

 autogen.sh      |    2 +-
 configure.ac    |    5 +-
 src/Makefile.am |    7 +-
 src/dfu.c       |   39 +++++-----
 src/dfu.h       |   23 +++---
 src/dfu_file.c  |  129 +++++++++++++++++++++++++-------
 src/dfu_file.h  |   24 +++---
 src/dfu_load.c  |   28 ++-----
 src/dfu_load.h  |    6 +-
 src/dfuse.c     |   64 ++++++++--------
 src/dfuse.h     |    6 +-
 src/dfuse_mem.c |    2 +-
 src/dfuse_mem.h |    6 +-
 src/main.c      |  120 +++++++++++++++---------------
 src/portable.h  |   22 ++++++
 src/quirks.c    |    5 ++
 src/quirks.h    |    3 +
 src/suffix.c    |  219 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/usb_dfu.h   |   54 ++++++--------
 19 files changed, 541 insertions(+), 223 deletions(-)
 create mode 100644 src/portable.h
 create mode 100644 src/suffix.c

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Stefan Schmidt-2
Hello.

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.

From my side we would be ready for a 0.6 release afterwards. :)

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Tormod Volden
On Sat, Apr 21, 2012 at 11:01 PM, Stefan Schmidt wrote:

>> ----------------------------------------------------------------
>> 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.

Yes, totally fine with me. I had just kept it in a separate commit
because the porting itself was kind of search and replace, and this
append/fseek "hack" was the only non-obvious thing.

So I have now squashed it and re-pushed, but kept the long explaining
commit messages.

(Of course, the first commit would not overwrite a user file unless
the user tried to overwrite it!)

>
> 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?

No, but I would guess everything non-posix. I believe, unless you are
using cygwin, the posix layer on Windows is not so complete. I know
that when compiling with MinGW, truncate() is not available. I just
found this http://stackoverflow.com/questions/584639/truncate-file so
it looks like we can use SetEndOfFile() on Windows instead. I will
look into that another day.

>
>>       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.

http://msdn.microsoft.com/en-us/library/windows/desktop/ms686298%28v=vs.85%29.aspx
says that you should include windows.h to use the Sleep() function. So
maybe not the preferred way to check for Windows but seems a good
guess for checking for Sleep().

>
>>       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.

Yes, that seems pretty random, but I estimated it to keep each string
below the standard max length, and I kind of grouped options together
logically. Another way would be to have printf's on every line...

>
>>       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.

Yes, but I did not add the text and delimiters around it either. I see
many tools use dots for this kind of progress. For now it makes it
easier for me to spot in a log if the right download option is being
used, so consistency is not a goal :) I can reconsider this later,
maybe we can use dots everywhere.

>
>>       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. :)

BTW, I have come to realize that we are getting no bug reports, so we
have to google for use of dfu-util and proactively discover the users'
problems. That's how I found the Maple fall-out!

>
>>       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. :)

Yes, they are not responsive. Since they are shipping dfu-util in
their product, I expected a little bit more of cooperation from them.

>
> 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.
>

Done!

> From my side we would be ready for a 0.6 release afterwards. :)

We could maybe wait to see if I can get suffix truncation work on
Windows easily, that would save some documentation and make dfu-util
fully consistent across architectures.

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Tormod Volden
On Sun, Apr 22, 2012 at 12:07 AM, Tormod Volden wrote:

> On Sat, Apr 21, 2012 at 11:01 PM, Stefan Schmidt wrote:
>>
>>>       dfu-suffix: Check for availability of truncate() function
>>
>> You happen to know which platforms are missing this?
>
> No, but I would guess everything non-posix. I believe, unless you are
> using cygwin, the posix layer on Windows is not so complete. I know
> that when compiling with MinGW, truncate() is not available. I just
> found this http://stackoverflow.com/questions/584639/truncate-file so
> it looks like we can use SetEndOfFile() on Windows instead. I will
> look into that another day.

I found ftruncate() on MinGW, so I changed to use that (with help of
fileno() to avoid reopening the file this got much cleaner too). For
non-MinGW Windows, it seems like _chsize_s is an equivalent (forget
about SetEndOfFile() which acts on Windows file handles). Since I can
not test that variant, and MinGW works fine, I am not including it.

>
>>
>>>       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.
>
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms686298%28v=vs.85%29.aspx
> says that you should include windows.h to use the Sleep() function. So
> maybe not the preferred way to check for Windows but seems a good
> guess for checking for Sleep().

Since we are using HAVE_WINDOWS_H it implies auto-tools is being used,
so it could seem like checking for Sleep() would be the proper way.
However that does not work when cross-compiling on MinGW. Similar to
the getpagesize() issue this might be a bug in auto-tools or I don't
know how to use it properly.

>> 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.
>>
>
> Done!

Done again, rebased with the ftruncate fix. Before repulling, do a
fetch so you can git diff the small difference.

>
>> From my side we would be ready for a 0.6 release afterwards. :)
>
> We could maybe wait to see if I can get suffix truncation work on
> Windows easily, that would save some documentation and make dfu-util
> fully consistent across architectures.


Should be fine now.

Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Tormod Volden
On Sun, Apr 22, 2012 at 11:02 AM, Tormod Volden wrote:

> On Sun, Apr 22, 2012 at 12:07 AM, Tormod Volden wrote:
>> On Sat, Apr 21, 2012 at 11:01 PM, Stefan Schmidt wrote:
>>>
>>>>       dfu-suffix: Check for availability of truncate() function
>>>
>>> You happen to know which platforms are missing this?
>>
>> No, but I would guess everything non-posix. I believe, unless you are
>> using cygwin, the posix layer on Windows is not so complete. I know
>> that when compiling with MinGW, truncate() is not available. I just
>> found this http://stackoverflow.com/questions/584639/truncate-file so
>> it looks like we can use SetEndOfFile() on Windows instead. I will
>> look into that another day.
>
> I found ftruncate() on MinGW, so I changed to use that (with help of
> fileno() to avoid reopening the file this got much cleaner too). For
> non-MinGW Windows, it seems like _chsize_s is an equivalent (forget
> about SetEndOfFile() which acts on Windows file handles). Since I can
> not test that variant, and MinGW works fine, I am not including it.

BTW, I squashed this commit into the "porting to stdio" commit, since
it ended up as a small change, and even more so combined (it was
originally using ftruncate, so otherwise it would be a lot of back and
forth between commits).

>>
>> We could maybe wait to see if I can get suffix truncation work on
>> Windows easily, that would save some documentation and make dfu-util
>> fully consistent across architectures.
>
>
> Should be fine now.

Tested and works fine on Windows 7.

Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Stefan Schmidt-2
Hello.

On Sun, 2012-04-22 at 11:50, Tormod Volden wrote:

> On Sun, Apr 22, 2012 at 11:02 AM, Tormod Volden wrote:
> > On Sun, Apr 22, 2012 at 12:07 AM, Tormod Volden wrote:
> >> On Sat, Apr 21, 2012 at 11:01 PM, Stefan Schmidt wrote:
> >>>
> >>>>       dfu-suffix: Check for availability of truncate() function
> >>>
> >>> You happen to know which platforms are missing this?
> >>
> >> No, but I would guess everything non-posix. I believe, unless you are
> >> using cygwin, the posix layer on Windows is not so complete. I know
> >> that when compiling with MinGW, truncate() is not available. I just
> >> found this http://stackoverflow.com/questions/584639/truncate-file so
> >> it looks like we can use SetEndOfFile() on Windows instead. I will
> >> look into that another day.
> >
> > I found ftruncate() on MinGW, so I changed to use that (with help of
> > fileno() to avoid reopening the file this got much cleaner too). For
> > non-MinGW Windows, it seems like _chsize_s is an equivalent (forget
> > about SetEndOfFile() which acts on Windows file handles). Since I can
> > not test that variant, and MinGW works fine, I am not including it.
>
> BTW, I squashed this commit into the "porting to stdio" commit, since
> it ended up as a small change, and even more so combined (it was
> originally using ftruncate, so otherwise it would be a lot of back and
> forth between commits).

Great, works fine here as well.

> >>
> >> We could maybe wait to see if I can get suffix truncation work on
> >> Windows easily, that would save some documentation and make dfu-util
> >> fully consistent across architectures.
> >
> >
> > Should be fine now.
>
> Tested and works fine on Windows 7.

Cool. I pulled it in, did a changelog, tagged it and now I'm in the
final testing before I push it all out (ETA 30min). I will wait with the
announcement for you to prepare the windows executable from either the
git tag or the release tarball. Docs about building for windows would
be a nice bonus, but for the release I only would like the windows
binary. Please include the version in the filename and also prepare a
.md5 file with the checksum. This way it can go straight to the
release folder with the tarball. Obviously take your time for this.
The final announcement can wait a day or two if you have more
important or better stuff to do. :)

regards
Stefan Schmidt


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu-util pull request

Stefan Schmidt-2
Hello.

On Sun, 2012-04-22 at 14:41, Stefan Schmidt wrote:
>
> Cool. I pulled it in, did a changelog, tagged it and now I'm in the
> final testing before I push it all out (ETA 30min).

No new problems showed up and I pushed it all out. The build server
building for Linux i386 and FreeBSD AMD64 was also happy:
http://jenkins.osmocom.org/jenkins/view/dfu-util/job/dfu-util/

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Loading...