Re: [gpsd-dev] Trying to refactor ntpshm.c

[Gary E. Miller]

Yo Eric!

So much for the release plan...

On Mon, 28 Oct 2013 08:25:07 -0400
"Eric S. Raymond" <address@hidden> wrote:

> Gary, I'm trying to refactor ntpshm.c, for three reasons:
> 
> 1) I want to use the PPS-monitoring loop in gpsmon.c so that it can
>    show PPS notifications in the packet window.  This means that the
>    pps-monitoring-thread machinery needs to be separated from the
> code for reporting to ntpd and chrony, which gpsmon won't use.

PPS is already in the JSON, so no biggee there.  chrony and ntpd are
two function calls so no bigge there either.  A restructure not needed.


> 2) Cleanup.  That module is very tangled.  By breaking it in two -
>    moving the thread management to a separate ppsthread.c - I think we
>    can increase the readability and maintainability of the code
>    substantially.

I think everything that is in the thread should be in one file.  What
has happened when you separate is people forget they are in a thread
and do not program thread aware.

> 3) Easier pluggability for other PPS reporting methods.  

So you add some pointers to functions.  Do not bother to optomize the
ntpd and chrony functions since I was just about to change the calling
conventions anyway for the upcoming ntpd changes.

> The relevant commits are:
> 
>   Begin the painful process of factoring the thread processing.
>   Attempting to disentangle the thread monitor  from ntpd and chrony
> reporting. More factoring out of chrony code.
>   Move some code and remove unneeded deoendency.
>   More code rearrangement.  No logic changes.
>   Comment polishing.
> 
> Please review these changes and test chrony operation.  If I broke
> anything, it was probably in "Attempting to disentangle...".

Well, chrony was broken last Thursday.  Maybe you fixed it.  

> 
> For the next steo, I need to know: will it break the intended
> semantics if I replace
> 
>               (void)ntpshm_pps(session, &tv);
> with
> 
>               (void)ntpshm_pps(session, &edge_tv);

Not sure what you mean about the semantics, but the functionality will
break.  tv != edge_tv and both need to be changed to ts.

> The reason I'm asking is that I want to factor this section of code
> 
>           TSTOTV( &tv, &ts );
>           if (session->ship_to_ntpd) {
>               log1 = "accepted";
>               if ( 0 <= session->chronyfd ) {
>                   log1 = "accepted chrony sock";
>                   chrony_send(session, &edge_tv, edge_offset);
>                 }
>               (void)ntpshm_pps(session, &tv);
>           } else {
>               log1 = "skipped ship_to_ntp=0";
>           }
> 
> into a hook function.  I'm confused by the way tv and edge_tv are
> managed; I can't tell whether they're reptesenting the same  thing
> or not.  (edge_tv used to be the tv component of sample.)

Wow, somebody broke that a while back.  chrony_send is supposed
to take a ts, not a tv.  Somebody tried to make chrony_send look
like ntpshm_pps() when they shoulda been going the otherway.

Since I was just about to change that, how about you wait until after I
fix it and change it to the new calling way.  

The nub is that the whole thing was originaly written to take uSec, but
then with KPPS and chronyd we needed nSec.  The conversion was not
complete and when done the code around tv and ts will be simpler.  Then
it looks like someone misunderstood and started mixing the two.

I sure wish I could make that file read only, things are always breaking 
in there...

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        address@hidden  Tel:+1(541)382-8588

signature.asc
Description: PGP signature