On 01-11-14 19:29, Linus Torvalds wrote:
On Fri, Oct 31, 2014 at 4:57 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
On Fri, Oct 31, 2014 at 3:25 PM, Jef Driesen jef@libdivecomputer.org wrote:
Proper integration with the libdivecomputer logging functions.
I'll take a look, today has been Linux work for me (and will be halloween later).
Ok, here's a re-send of the libdivecomputer patches (no change to the subsurface part, so I'm not resending that patch). The two DC_FIELD_STRING patches haven't really changed, but they aren't identical either, due to changes around them.
This - once again - squashed all the changes into one "EON Steel support patch": your three patches and the change to use the libdivecomputer ERROR() function. It's still wrapped with the "report_error()" function due to return value issues (returning DC_STATUS_SUCCESS on error? That's just odd, but regardless, the internal EON steel functions return number of bytes transmitted etc, so the enums just wouldn't work even if they were actually error numbers),.
Just a quick comment, because I'm running out of time right now.
I still prefer to have proper integration of the logging functions. Calling ERROR from your report_error is an ugly hack that throws away several of the nice features of the ERROR macro. For example the line numbers and function names will always be those of your report_error() function, hiding the real source. And when building with all logging disabled, your report_error function is still being called. Your debug() function is still there too. Basically I want all printf's (directly or through a wrapper function) replaced.
Also, my patch with the casts was merely to indicate where the problems were, and not supposed to be applied as is. Ideally I would rather see the root problem "fixed" instead of fixing the symptoms.
For example by changing dive_directory to an array:
-static const char *dive_directory = "0:/dives"; +static const char dive_directory[] = "0:/dives";
and then calling memcpy instead of strcpy:
- strcpy((char *)cmd+4, dive_directory); - cmdlen = 4+strlen(dive_directory)+1; + memcpy(cmd + 4, dive_directory, sizeof(dive_directory)); + cmdlen = 4 + sizeof(dive_directory);
Saves a strlen call too :-)
This is just an example, but I think you'll understand what I mean.
If you'd prefer an incremental diff instead, just holler. But I rebased on top of your tree, and you hadn't applied the patches, so I was assuming you want a clean resend.
A clean resend is what I prefer as well. These incremental changes don't add much value to the git history.
Jef