On 2017-06-02 18:26, John Van Ostrand wrote:
On Fri, Jun 2, 2017 at 5:18 AM, Jef Driesen jef@libdivecomputer.org wrote:
Can you reformat your commit message to follow the standard git format? You can find a very nice description here:
It's been a while since I used git and it became clear after doing a couple commits that I forgot how commit messages appear.
No problem at all. It takes some learning.
Anyway, with git you can very easily modify commits afterwards (commit message and/or content) to submit an updated patch.
git rebase --interactive git commit --amend
// Validateif (sample_start_address <device->layout->rb_profile_begin ||
sample_start_address >device->layout->rb_profile_end ||
sample_end_address <device->layout->rb_profile_begin ||
(sample_end_address >device->layout->rb_profile_end &&
sample_end_address != 0xFFFFFFFF)) {continue;}Why did you remove the validation? Now you are using data that is potentially bogus. That's a bad idea!
The cochran_commander_profile_size() function returns sample_size of 0 if these values are bad. If the sample_size is 0 we don't use the data. Although I notice that in one case it will alter sample_end_address, so I added that to the foreach function too.
You are right. I didn't notice that, and that's exactly why I still don't like this construct. I prefer to do the validation right away. That way, the data is always safe to use. Now, you'll run into problems if there is a code path where the function isn't called.
BTW, I usually also prefer to report errors instead of silently ignoring them. It's might be a bit less convenient for the end-user that runs into the problem, but bug fixing becomes easier. Because that makes the difference between a bugreport saying "something looks wrong" versus "it fails with error X (at line Y in file Z)".
do {
rc = cochran_commander_read(device, &progress, sample_start_address, dive + device->layout->rb_logbook_entry_size, sample_size);
} while (rc != DC_STATUS_SUCCESS &&tries++ < 3);
What's the reason for the retrying here? You don't do it elsewhere (for example when reading the logbook, or from the cochran_commander_device_{read,dump} functions), so I wonder what's different here.
Reading from the Cochran at the high baud rates isn't reliable. Very large reads (multiple MB) result in errors about 1/3rd of the time. This is reason why I decided to read each dive profile individually. Small reads are more reliable but when reading hundreds of times it's likely a read error will occur.
In that case you should integrate the retrying in the cochran_commander_read() function itself (or some extra helper function). Then you no longer have to duplicate the retrying code everywhere the function is being called.
if (rc != DC_STATUS_SUCCESS) {
ERROR (abstract->context,"Failed to read the sample data.");
return rc;Again a memory leak here! There are two similar ones a few lines below.
Fixed.
There is still memory leak left. The "dive" pointer isn't freed when you jump to the error handler.
Jef