Re: gpxlogger (x: ✘3.23 is near. Please test!)

[Fred Wright]

On Fri, 30 Jul 2021, Gary E. Miller wrote:
> On Thu, 29 Jul 2021 15:59:31 -0700 (PDT)
> Fred Wright <fw@fwright.net> wrote:
>
> > On Mon, 26 Jul 2021, Gary E. Miller wrote:
> >
> > > I want to get gpsd 3.23 shipped, due to issue #144.
> > >
> > > Please test git head.  Unless a show-stopper bug pops up, I'd like
> > > to get 3.23 shipped by Aug 2.  Version 3.22 was shipped Jan 8,
> > > 2021.
> >
> > I haven't done any multi-platform testing yet, but I did find that
> > gpxlogger has become unquittable, due to a863161f8.
>
> I think fixed in commit c9cdf7cba1bf87ae88f756b7dd63b133b12bd3f1

Partially - see below.

> > Previously, quit_handler() was doing things which were technically
> > illegal but worked, including a gps_close() which terminated the
> > gps_mainloop(). The new code seems to assume that gps_mainloop()
> > returns frequently, which it doesn't, since it's designed to iterate
> > internally, and the only reason for the outer loop was for
> > reconnecting.  Adding the sig_flag check there doesn't help, because
> > it's never reached.
>
> Agreed.  So I put the exit in the mainloo callback.  Then added an atexit()
> to send the gpx footer.

That works in the common case, but not in the case where the daemon isn't 
sending anything.  I don't see any way to fix that without some change to 
the API, putting back the ugly gps_close() in the signal handler, or 
directly exiting from the signal handler (which is what it did way back 
when).

> Needs more work, but after release.

Fixing it properly can wait, but I verified that the 3.22 version does 
handle this correctly, so it's indeed a regression as is.

> > The real problem is the bad design of gps_mainloop(), which provides
> > no way to stop its iterations other than by error or timeout.  Aside
> > from the whole signal handler issue, relying on something like
> > gps_close() to stop it is ugly and probably not thread-safe.  The
> > obvious fix is to have the hook function return a value which
> > indicates whether iterations should continue, perhaps with a
> > different return value from gps_mainloop() itself to distinguish
> > "stop iteration" from "error".  However, this would change the API.
> > Currently, the only use of gps_mainloop() within the project is in
> > gpxlogger, though it's not known whether users are using it.  And
> > testing the return value of a function declared void would lead to
> > flaky behavior.  The conservative thing to do would be to introduce a
> > new version of gps_mainloop() under a different name, and keep the
> > broken one (probably a wrapper around the good one) around for a
> > while for compatibility.
>
> I added some comments that it needs to be fixed.  I think the API
> can be saved by jsut extending it.  After release.

Obviously my suggestion for the return value wouldn't work when the hook 
isn't getting called at all. :-) Needs more thought.

Fred Wright