fix reliability of print_link_stats (with option persist) #505
Conversation
enometh
commented
Jul 14, 2024
enometh
commented
- pppd/ipcp.c: (ipcp_down): fix comment
- pppd/main.c: (reset_link_stats): reset print_link_stats to 1, set start_time even if get_ppp_stats fails.
|
This is an attempt to fix the problem noted in the linux-ppp mailing list on mar-26-2024 and may-03-2024 The sent/recv log messages were being lost, especially with the persist option. it also fixes a stray reference to the old variable in a comment. |
|
@paulusmack: Have you seen this @enometh PR? |
|
First, this needs a sign-off, see Submitting-patches.md. Secondly, it needs an explanation that is more than simply saying what the textual change is. Don't make the reader have to reverse-engineer your thought process. |
I explained the change to the extent it could be explained both on the mailing list and in the comment on the pull request. Since the patch was a trivial 2 line bugfix, of someone else's code, I skipped looking into what the "Sign off" involved. However I am using my real name and real email address. (I go by S.Madhu, officially) |
enometh
force-pushed
the
fix-stat-printing-take-1
branch
from
7bf6828
to
bc798c1
Compare
|
Not sure why the DCO check is failing... |
|
I'd prefer if we filed a bug on what wasn't working. All it says is it is a reliability issue with signal 2?? The get_ppp_stats() function returns 1 on success, then it also configures a timer to call a function repeatedly. The function it calls depends on what is configured and what OS you are on. Debug logs should reveal what is going on here. The get_ppp_stats() returns the value of the function it dynamically resolved, and if success it should be "1". If you get "0" then I'd be very interested in why?? So, again; please file a bug and we can look at your setup and try to figure out what is going on wrong. I am not sure if simply adding a few lines to say "now" it works, to resolve "what" problem in the first place. |
Please see I believe this is a bonafide bug in the code which my patch fixes, and would appreciate if someone |
* pppd/ipcp.c: (ipcp_down): fix comment * pppd/main.c: (reset_link_stats): reset print_link_stats to 1, set start_time even if get_ppp_stats fails. This is an attempt to fix the problem noted in the linux-ppp mailing list on mar-26-2024 and may-03-2024 under the subject "ppp-2.5.0 sometimes doesn't print stats on terminating on signal 2" The sent/recv log messages were being lost, especially with the persist option. This seems to be an oversight during reorg in commit ba7f7e0 "Header file reorganization and cleaning up the public API for pppd version 2.5.0 (ppp-project#379)" around the repurposing of the link_stats_valid variable as link_stats_print. It also fixes a stray reference to the old variable in a comment. please review the reordering in reset_link_stats Signed-off-by: S Madhu <[email protected]>
enometh
force-pushed
the
fix-stat-printing-take-1
branch
from
bc798c1
to
e0cce55
Compare
|
|
@enaess I think the basic problem is that link_stats_print is only ever set to 1 on initialization, hence print_link_stats() will only ever produce output once (at most). However, there are cases, e.g. when the persist option is used, where we used to get output from print_link_stats more than once, as the one pppd instance could handle a series of connections. This change was introduced in ba7f7e0. The fix proposed here, to set print_link_stats to 1 in reset_link_stats(), seems reasonable to me. I agree it wants a bit more explanation in the commit message - the fix may be textually simple but that doesn't mean that the reasoning behind it is intuitively obvious. |
| @@ -1331,9 +1331,9 @@ print_link_stats(void) | |||
| void | |||
| reset_link_stats(int u) | |||
| { | |||
| if (!get_ppp_stats(u, &old_link_stats)) | |||
| return; | |||
| get_ppp_stats(u, &old_link_stats); | |||
There was a problem hiding this comment.
Should we initialize old_link_stats to all zeroes if the get_ppp_stats call fails? Or copy it from link_stats?
|
I think the use of print_link_stats was intended to avoid double printing the result on exit (though, I am not 100% sure). If there was instances where an update was to be dispatched to a script on a regular interval, then I must have missed that memo. I'd have to run this code and troubleshoot any side-effects of making that change. I do recall it wasn't a straight forward shot. Also, do we know if the print_link_stats will ever be called in case of a SIGHUP (and if it is, the value of print_link_stats is invalid?). |
|
* Eivind Næss ***@***.***> ***@***.***>
Wrote on Wed, 21 Aug 2024 09:59:26 -0700
I *think* the use of print_link_stats was intended to avoid double printing the result on exit (though, I am not 100% sure). If there was instances where an update was to be dispatched to a script on a regular interval, then I must have missed that memo.
I think the patch preserves the logic to avoid double printing the
result.
I'd have to run this code and troubleshoot any side-effects of making that change. I do recall it wasn't a straight forward shot. Also, do we know if the print_link_stats will ever be called in case of a SIGHUP (and if it is, the value of print_link_stats is invalid?).
I haven't rechecked but I believe the code tries to say: SIGHUP is
supposed to cleanly terminate ppd and print the stats reliably. This
was not a problem earlier.
|
|
* Paul Mackerras ***@***.***> ***@***.***>
Wrote on Wed, 21 Aug 2024 02:29:19 -0700
void
reset_link_stats(int u)
{
- if (!get_ppp_stats(u, &old_link_stats))
- return;
+ get_ppp_stats(u, &old_link_stats);
Should we initialize old_link_stats to all zeroes if the get_ppp_stats call fails? Or copy it from link_stats?
I don't know because I couldn't imagine a situation where
get_ppp_stats would fail (and you'd need to know the situation to
decide if the state is to be preserved or blown away). to quote a
fragment in a comment upthread:
"The get_ppp_stats() returns the value of the function it
dynamically resolved, and if success it should be "1". [If you
get "0" then I'd be very interested in why??]
I proposed the change because I thought get_ppp_stats is always
expected to succeed here, and returning immediately is the wrong thing
to do for this function. the failure would probably be catastrophic
but it would "handled" elsewhere. and there are other calls in the
code where the return value of get_ppp_stats is not checked, so
leaving things as were was probably the best bet,
|
|
I don't recall correctly, but didn't @paulusmack fix-up some of the signal handling in pppd in the last release (or was that chat.c?). You reported seeing the stats being printed in some cases, and in some cases not? Perhaps the signal handling code has shifted things around that it introduced this bug. I'd love getting a repro on this and test this out, but I don't have the time to fiddle with this issue right now. |
The key thing here that maybe you missed is that with the persist option, the one pppd instance can handle more than just one connection lifecycle. That is, with persist, pppd will try to reconnect after the connection terminates, and a given pppd instance could handle an unlimited number of connections (in serial fashion, one after another). So the logic where print_link_stats is set to 0 when the stats are printed and never set to 1 again after that is wrong. It's nothing to do with signals. |
