[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [gpsd-dev] Cleanup patches for the mingw port
From: |
ukyg9e5r6k7gubiekd6 |
Subject: |
Re: [gpsd-dev] Cleanup patches for the mingw port |
Date: |
Thu, 16 May 2013 13:28:44 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:10.0.12) Gecko/20130116 Icedove/10.0.12 |
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
- Re: [gpsd-dev] Cleanup patches for the mingw port,
ukyg9e5r6k7gubiekd6 <=