Re: [gpsd-dev] Cleanup patches for the mingw port

[ukyg9e5r6k7gubiekd6]

Hi Eric and GPS devs,

Firstly, please accept my apologies for disappearing. There is no 
excuse, but Real Life is the reason.
I've replied to your points inline below - I hope you don't mind. 
There's a summary at the end.

On 07/09/12 20:34, Eric S. Raymond wrote:
> Here are the results of my attempts to apply the first round of mingw
> patches.
>
> 0001-Rename-add_device-to-gpsd_add_device-and-remove-stat.patch
> 0002-Move-nowait-out-of-gpsd_add_device.patch
> 0003-Invent-NOWAIT-macro.patch
> 0004-Add-can_net-to-driver.nmea2000-structure.patch
> 0005-Add-framework-for-multiple-units.patch
>
>         These look like Reinhardt's work on the CAN support,
>         included on the cleanup branch by mistake.
Yep, those are the result of an unwise merge. I see you disregarded 
them. Sorry, still learning how to use git.
> 0006-Ignore-Eclipse-project-files.patch
>
>         Applied.
>
> 0007-Remove-ugly-intptr_t-casts-from-gpsdata-gps_fd.patch
>
>         I agree those casts are ugly, but without them one of our
>         auditing tools gets confused.  Not applied, but we can discusss
>         this further.
Which auditing tool did these confuse? Maybe we could find some other 
way to make it happy.
I didn't observe any additional splint warnings from these. I may not 
(unsure) have run this particular change via cppcheck, as there was a 
while back there where cppcheck was crashing for me every time.
If it's possible, I'd still like to remove these casts - but obviously 
not at cost of introducing auditing problems.
> 0008-Replace-tests-of-value-of-gps_fd-which-thus-assumed-.patch
>
>         Failed to apply cleanly to libgps_core.c.  Please resubmit
>         against head.  Actually, what I'd like is a patch that
>         does all the GOODSOCK/BADSOCK changes in one go.
I think you've now performed most or all of the GOODSOCK changes, so I 
doubt there are any direct tests of the value of gps_fd left. I'll 
perform a quick pass over the latest source to check. Do you know of any 
leftovers here?
> 0009-Placate-splint-by-adding-U-suffix-to-unsigned-intege.patch
>
>         splint wasn't complaining for me, but I've applied
>         a slightly modified version of this patch (u suffixes
>         rather than U) because it looks like a good cleanup.
Oops. Thanks.
> 0010-Silence-some-more-splint-warnings-and-use-GOOD-BAD-S.patch
>
>         Partly applied. I omitted the BADSOCK application.
>         I wasn't seeing splint warnings from this code, which
>         makes me wonder why you were.
I'm using splint from Debian unstable, on both i386 and amd64. What are 
you using?
Unfortunately I don't remember exactly which version(s) of splint I was 
using at the time I wrote this patch. Right now I have 3.1.2.
Incidentally, I know BADSOCK is rather pointless, but I included it 
solely to avoid ! operators in conditionals, which I'm told hurt 
readability slightly. But this additional macro also hurts readability, 
and it's arguable which is worse.
On reflection, I really don't think it matters much either way.
> 0011-Placate-splint-by-conditionally-not-declaring-defini.patch
> 0012-Replace-more-numeric-comparisons-of-gpsdata-gps_fd-w.patch
> 0013-More-replacements-of-numeric-comparisons-of-sockets-.patch
> 0014-More-GOOD-BADSOCK-use.patch
> 0015-The-last-I-think-of-GOOD-BADSOCK-usage.patch
>
>         Not applied; I'd like to see one patch that rolls up all
>         of these, 0008, 0031, and the BADSOCK parts of 0010/0016/0025.
I believe that out of these, only 011 remains. So I'll submit that as a 
patch.
> 0016-Use-socket_t-rather-than-int-and-GOOD-BADSOCK-rather.patch
>
>         int to socket_t changes applied.
>
> 0017-Remove-use-of-struct-termios-from-public-API-of-seri.patch
>
>         Applied.
>
> 0018-sockaddr_t-only-needs-to-accomodate-a-sockaddr_in6-i.patch
>
>         Applied.
>
> 0019-Use-UNUSED-rather-than-uselessly-setting-a-parameter.patch
>
>         Applied.
>
> 0020-Silence-some-pointer-signedness-warnings.patch
>
>         Applied.
>
> 0021-Silence-cppcheck-warning.-Limits-year-values-to-10-9.patch
>
>         Applied.
>
> 0022-Silence-splint-and-cppcheck-mostly-by-reducing-varia.patch
>
>         Patch failed. Please resubmit.
I believe you've recently fixed some at least of these; but I'll merge 
these changes into head and submit a patch in case some remain.
> 0023-Comply-stricly-with-strict-aliasing-rules-by-using-m.patch
>
>         Applied.  Good catch!
>
> 0024-Silence-another-cppcheck-warning.patch
>
>         Applied.
>
> 0025-Instead-of-passing-errno-via-gps_fd-ensure-netlib_co.patch
>
>         Not applied.  I want to see the parts relating to GOODSOCK/BADSOCK
>         split apart from the errno juggling.
Since I think GOODSOCK is now bedded down, is it now time to re-visit 
this errno juggling?
To be frank, I am not a big fan of this approach - global variables like 
errno peeve me.
But that's not our fault, and using errno overtly is better than using 
it covertly, disguised as an fd.
> 0026-Use-strerror-rather-than-a-generic-error-message.-No.patch
> 0027-Make-libgps_core.c-s-extern-functions-leave-errno-wi.patch
> 0028-Use-errno-for-diagnostics-rather-than-ab-using-socke.patch
> 0029-Leave-errno-with-a-meaningful-value-on-return.patch
>
>         Not applied, as these look dependent on 0025 to work.
>         I'd like to see a all the errno juggling consolidated
>         into one patch with a good explanation of how this changes
>         the library's expected behavior.
Understood. Yes, these are really all part of "project errno", so I'll 
lump them in with 0025 and submit a patch. As long as you are OK with 
the approach generally, that is (which it sounds like you are).
> 0030-Use-exit-EXIT_-SUCCESS-FAILURE-rather-than-0-1-respe.patch
>
>         Applied.
>
> 0031-More-GOOD-BADSOCK.patch
>
>         Not applied.  I want to see this as part of a consolidated
>         patch to do that change everywhere.
Again I think you've now effectively made and applied such a 
consolidated patch (and I'm sorry I didn't do that myself). Can you 
confirm/deny?
> 0032-Remove-unnecessary-includes-of-termios.h.patch
>
>         Applied.
Thanks for applying. This was a fairly major stumbling block to Win32 
support - pages of missing header errors is not a pretty thing for the 
would-be porter to see!
> 0033-Replace-uint-with-unsigned-everywhere-obviating-one-.patch
>
>         Not applied.  I'll take an equivalent patch that consistently
>         uses 'unsigned int', as opposed to bare 'unsigned'.
Okay, adding that, too to the list of cleaned-up patches I owe you.
From memory, 'unsigned' caused nicer whitespace alignment/indenting 
than 'unsigned int', that's all.
> 0034-Compile-most-code-with-Werror.patch
>
>         Applied.  I am quite pleased thgat this didn't break the build.
Well, yes, I saw the later history of this.
I think at least you have a right to be proud that it didn't break 
things worse than it did...
> 0035-Use-unsigned-short-instead-of-ushort-obviating-a-typ.patch
>
>         Applied.
>
> 0036-Remove-trailing-whitespace-from-C-and-C-source.patch
>
>         Partly applied; there were a few failures. Feel free
>         to submit a supplementary patch.
>
> 0037-Silence-splint-warning.-Regression-tests-pass.patch
>
>         Applied.

In summary, patches remaining that I'm to submit are:

1. 0007-Remove-ugly-intptr_t-casts-from-gpsdata-gps_fd.patch
But without introducing the audit regression(s) to be described by Eric.

2. 0008-Replace-tests-of-value-of-gps_fd-which-thus-assumed-.patch
If there are any of them left, of course.

3. 0011-Placate-splint-by-conditionally-not-declaring-defini.patch

4. 0022-Silence-splint-and-cppcheck-mostly-by-reducing-varia.patch a
If anything remains to be done.

5. "Project errno"
Incorporating at least:

0025-Instead-of-passing-errno-via-gps_fd-ensure-netlib_co.patch
0026-Use-strerror-rather-than-a-generic-error-message.-No.patch
0027-Make-libgps_core.c-s-extern-functions-leave-errno-wi.patch
0028-Use-errno-for-diagnostics-rather-than-ab-using-socke.patch
0029-Leave-errno-with-a-meaningful-value-on-return.patch

In case it's not obvious, my intention is to satisfy:
5.1. If anything wants to get at an errno value, it does so via errno 
itself, not via something else whose value we hope was set to errno at 
some sufficiently-recent time in the past; and
5.2. All  'interesting' functions should leave errno in a valid state, 
reflecting the success or failure of the work the function attempted. 
One day I would like it if every function left errno in a valid state.
5.3. Every piece of code that sets errno on an error, shall also set 
errno to a valid value in its success case. This way the calling code 
either always or never uses errno.
I think rules like 'you must only look at errno in an error path' are 
extremely error-prone. Also they can lead to really dumb behaviour, eg 
schizophrenic messages like "An error occurred: Success".

6. 0033-Replace-uint-with-unsigned-everywhere-obviating-one-.patch
But use 'unsigned int' rather than 'unsigned'. C source is not ASCII art :-)

7. 0036-Remove-trailing-whitespace-from-C-and-C-source.patch

Did I miss anything?

Also Eric, could you please elaborate on the need for the intptr_t casts?

Regards,

Matt