By my mistake (gmail defaults to Reply-to) the below did not get to
list. New version of the patch will follow.
Tormod
On Tue, May 24, 2011 at 8:55 AM, Stefan Schmidt wrote:
> Hello.
>
> On Mon, 2011-05-23 at 23:11, Tormod Volden wrote:
>> On Mon, May 23, 2011 at 7:39 PM, Stefan Schmidt wrote:
>> >> The commit also includes some verification of the suffix fields,
>> >> but at this point only warns about mismatches, with one exception:
>> >> If there is a valid DFU suffix, it must have DFU revision 1.0,
>> >> otherwise we abort the download.
>> >
>> > Thats because we are only supporting version 1.0 of the spec right
>> > now? If yes, thats fair enough and we can adjust it once 1.1 support
>> > has landed.
>>
>> Actually, the 1.1 spec still says: "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 if that is not a typo, checking for 0x100 covers both 1.0 and 1.1.
>
> Right, some other protocol numbers got changed and I mixed that up.
>
>> >> + * (C) 2011 Tormod Volden <debian.tor...@gmail.com>
>> >> + */
>> >
>> > Please add the GPL header here. We are using it in all c files but
>> > normally don't care in really small header files.
>>
>> Sure.
>
> Thanks
>
>> >> +/* reads the fd and name member, fills in all others */
>> >> +/* returns 0 if no DFU suffix */
>> >> +/* returns positive if valid DFU suffix */
>> >> +/* returns negative on file read error */
>> >
>> > I normally prefer multiline comments just start with /* on the first
>> > line and terminate with */ on the last line. But thats just
>> > nitpicking.
>>
>> I agree that will look better here.
>
> Good
>
>> >> +int parse_dfu_suffix(struct dfu_file *file)
>> >> +{
>> >> + int ret;
>> >> + struct stat st;
>> >> + /* supported suffices are at least 16 bytes */
>> >> + unsigned char dfusuffix[16];
>> >> +
>> >> + file->size = 0;
>> >> + /* default values, if no valid suffix is found */
>> >> + file->dwCRC = 0;
>> >> + file->suffixlen = 0;
>> >> + file->bcdDFU = 0;
>> >> + file->idVendor = 0xffff; /* wildcard value */
>> >> + file->idProduct = 0xffff; /* wildcard value */
>> >> + file->bcdDevice = 0xffff; /* wildcard value */
>> >> +
>> >> + ret = fstat(file->fd, &st);
>> >> + if (ret < 0) {
>> >> + perror(file->name);
>> >> + return ret;
>> >
>> > Looks to me like the indent is broken here. This happens again and
>> > again below.
>>
>> Hmm, I will have to check that. BTW, are you against using 4-spaces
>> indentation (used consistently in single files)?
>
> I have a strong feeling for tabs here. And the Linux kernel coding
> style in general. In the ends its just a style and I would not like to
> discuss such things to much I strongly believe that it needs to be
> consistent within a project though.
>
>> >> + file->suffixlen = dfusuffix[11];
>> >> + if (file->suffixlen < sizeof(dfusuffix)) {
>> >> + fprintf(stderr, "Unsupported DFU suffix length %i\n",
>> >> file->suffixlen);
>> >> + ret = 0;
>> >> + goto rewind;
>> >> + }
>> >
>> > I'm wondering if it is worth it to read out the suffix length from the
>> > suffix itself as it has a fixed size in both 1.0 and 1.1. On the other
>> > hand it is provided in the suffix we should read it from there for the
>> > sake of completeness? :)
>>
>> This is for the sake of future compatibility. If new
>> standards/revisions come along, the fields we already pick out will
>> still be valid, but the suffix might have been extended. This makes
>> sure we cut of the right amount when downloading, and it might even
>> work without having to change the code.
>
> I don't see a new revision coming along after nearly 7 years but yes
> its easy enough to read the provided value so we should go with it.
>
>> >> + file->idVendor = (dfusuffix[5] << 8) + dfusuffix[4];
>> >> + file->idProduct = (dfusuffix[3] << 8) + dfusuffix[2];
>> >> + file->bcdDevice = (dfusuffix[1] << 8) + dfusuffix[0];
>> >> +
>> >> +rewind:
>> >> + lseek(file->fd, 0, SEEK_SET);
>> >> + return ret;
>> >> +}
>> >
>> > The major part that is missing here is the check of the CRC sum over
>> > the file. As we discussed earlier I can handle this after this
>> > patchset got merged.
>>
>> Yes, this can be implemented pretty cleanly now. I think it is less
>> important. For instance a verification option by reading back from the
>> device would be more useful. Both would be awesome :)
>
> I will work on CRC for the n+1 release. Reading back from the device
> for verification could be interesting. Needs to go on the TODO list.
> :)
>
> regards
> Stefan Schmidt
>
_______________________________________________
devel mailing list
devel@lists.openmoko.org
https://lists.openmoko.org/mailman/listinfo/devel
