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

[Eric S. Raymond]

ukyg9e5r6k7gubiekd6 <address@hidden>:
> >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 think it was cppcheck.

> >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?

I don't know of any leftovers.

> >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?

3.1.2, I think.

> 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).

I am very unlikely to make incompatible changes to the library API
at this point.  But I'd like to see your patch and consider it.

> >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?

Not easily.  I've forgotten what was in that patch.

> 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?

That sounds like a good program.  Try to do everything except 1 and 5 first;
those may be messy.  I want 5.2 and 5.3, but I'm going to scrutinize any
attempt at 5.1 very closely because I want to avoid an API compatibility
break.
 
> Also Eric, could you please elaborate on the need for the intptr_t casts?

I wish I could, but I don't remember where the issue was. Let's cross that
bridge when we get to it.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>