[dfu-util] New dfu-suffix manipulation tool

[Stefan Schmidt]

Hello.

Thanks for taking some time looking into this.

On Thu, 2012-03-01 at 11:45, Patryk Benderz wrote:
>
> I didn't compile this, just looked on logical consistency with DFU
> specification I was able to find [1]. If this is not proper one, please
> correct me. 

The USB forum has the specs available:

www.usb.org/developers/devclass_docs/usbdfu10.pdf
www.usb.org/developers/devclass_docs/DFU_1.1.pdf

That are the ones I use.

> First what I have noticed is different bcdDFU field. You have hard-coded
> 0x0100(=0100h, right?), while [1] document is labeled as DFU_1.1.pdf. So
> the title of file mentions next minor version number. However in
> appendix B (page 41) it is written:
> "The bcdDFU field is a two-byte specification revision number. The value
> as of this revision of the specification is 0100h, representing version
> 1.0."
> so the document [1] seems to be inconsistent with itself. Shouldn't this
> be clarified with usb.org , before hard coding bcdDFU?
> 
> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix&id=15757085aa10294b9a58719a42e922fe30405ff8
> 1.I am not skilled programmer, but I was taught that constant values
> like bcdDFU or bLength, in code should be in #define declaration... if
> it makes any difference, apart from good practices.

Well, good practices is in the eye of the beholder. :)

Why should they be in a define? The compiler detects if the variables
are constant and optimizes this case anyway. For the case that one
should avoid magic values like 16 here with a named constant I would
agree. I was just to lazy for doing it here. :)

> 2.I could not find ucDfuSignature field, or similar one anywhere.

Its in dfusuffix[8,9,10]. Written in dfu_file.c line 217-219.

> 3.Order of filling dfusuffix[] matrix is different from what Appendix B
> says. This indeed might indicate that you have used different version of
> DFU documentation than 1.1. If that is true, do you consider adapting
> your patch to fit 1.1 DFU version?

Appendix B is identical for version 1.0 and 1.1. I think you might be
confused with the negative offset given in the table. Let me quote the
spec here:

"The offsets are negative. This is a file suffix, and the negative
offsets indicate that the last byte of the file is specified in the dwCRC field"

Considering this the bytes are appended to the file in the correct
order. Other tools have been able to decode a suffix I added with this
code.

> 4. And again, do not take it personal, but I was taught to avoid 'goto'
> directive, as compilers do not like it. Additionally this kind of
> directives are not elegant. So maybe instead using "rewind:" label, it
> could be written this way (intentionally left a lot of new lines to ease
> reading)"
> 
> +ret = lseek(file->fd, 0, SEEK_END);
> +if (ret < 0)
> +{
> +  fprintf(stderr, "Could not seek to DFU suffix\n");
> +  perror(file->name);
> +}
> +else
> +{
> +  /* After seeking to the end of the file add the suffix */
> +  ret = write(file->fd, dfusuffix, sizeof(dfusuffix));
> +  if (ret < 0)
> +  {
> +    fprintf(stderr, "Could not read DFU suffix\n");
> +    perror(file->name);
> +  } 
> +  /* Inserting "ret >=0 && " to avoid entering statement when ret<0 */
> +  else if (ret >=0 && ret < sizeof(dfusuffix))
> +  {
> +    fprintf(stderr, "Could not read whole DFU suffix\n");
> +    ret = -EIO;
> +  }
> +}
> +
> +lseek(file->fd, 0, SEEK_SET);
> +return ret;
> +}

And looking at it I know why goto was chosen here. All this if else
and testing of different values makes the code not cleaner in my
opinion. Handling failure cases during initialization is actually the
only use case I use goto for. :)

Maybe I'm biased due to some kernel work here but in my opinion it
looks cleaner and is easier to understand instead of repeating the
failure code all the time. Its a matter of coding style in the end.

> Just a final question about version of DFU. Did you used 1.0 or 1.1?
> Will try to look at another commit later.

I mainly use the DFU 1.0 spec. There are not much differences in the
1.1 spec though.

Thanks again for your comments.

regards
Stefan Schmidt