Skip to content

Commit 758f5d7

Browse files
rhansenrhansen
committed
Fix handling of legacy status value
When a legacy protocol implementation returns, move its `status` and `ip` results to the new `status-ipv4` and `ipv4` (or `status-ipv6` and `ipv6`) properties. Also remove the now-unused `status` variable definition, and remove `ip` from the recap.
1 parent 11a3b4d commit 758f5d7

File tree

3 files changed

+109
-118
lines changed

3 files changed

+109
-118
lines changed

‎ChangeLog.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ repository history](https://github.com/ddclient/ddclient/commits/master).
133133
[#734](https://github.com/ddclient/ddclient/pull/734)
134134
* Fixed unnecessary repeated updates for some services.
135135
[#670](https://github.com/ddclient/ddclient/pull/670)
136+
[#732](https://github.com/ddclient/ddclient/pull/732)
136137
* Fixed DNSExit provider when configured with a zone and non-identical
137138
hostname. [#674](https://github.com/ddclient/ddclient/pull/674)
138139
* `infomaniak`: Fixed frequent forced updates after 25 days (`max-interval`).

‎ddclient.in‎

Lines changed: 65 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ my $saved_recap;
158158
my %saved_opt;
159159
my $daemon;
160160
# Control how many times warning message logged for invalid IP addresses
161-
my (%warned_ip, %warned_ipv4, %warned_ipv6);
161+
my (%warned_ipv4, %warned_ipv6);
162162

163163
sub repr {
164164
my $vals = @_ % 2 ? [shift] : [];
@@ -633,16 +633,20 @@ our %variables = (
633633
'max-interval' => setv(T_DELAY, 0, 0, interval('25d'), 0),
634634
'min-error-interval' => setv(T_DELAY, 0, 0, interval('5m'), 0),
635635

636-
# As a recap value, this is the IP address (IPv4 or IPv6, but almost always IPv4) most
637-
# recently saved at the DDNS service. As a setting, this is the desired IP address that
638-
# should be saved at the DDNS service. Unfortunately, these two meanings are conflated,
639-
# causing the bug "skipped: IP address was already set to a.b.c.d" when the IP was never
640-
# set to a.b.c.d.
641-
# TODO: Move the recap value elsewhere to fix the bug.
642-
'ip' => setv(T_IP, 0, 1, undef, undef),
643-
# As `ip`, but only IPv4 addresses.
636+
# The desired IP address (IPv4 or IPv6, but almost always IPv4) that should be saved at the
637+
# DDNS service.
638+
# TODO: Legacy protocol implementations write the IP address most recently saved at the
639+
# DDNS service to this variable so that it can be saved in the recap (as `ipv4` or `ipv6`).
640+
# Update the legacy implementations to use `ipv4` or `ipv6` instead, though see the TODO
641+
# for those variables.
642+
'ip' => setv(T_IP, 0, 0, undef, undef),
643+
# As a recap value, this is the IPv4 address most recently saved at the DDNS service. As a
644+
# setting, this is the desired IPv4 address that should be saved at the DDNS service.
645+
# Unfortunately, these two meanings are conflated, causing the bug "skipped: IP address was
646+
# already set to a.b.c.d" when the IP was never set to a.b.c.d.
647+
# TODO: Move the recap value elsewhere to fix the bug.
644648
'ipv4' => setv(T_IPV4, 0, 1, undef, undef),
645-
# As `ip`, but only IPv6 addresses.
649+
# As `ipv4`, but for an IPv6 address.
646650
'ipv6' => setv(T_IPV6, 0, 1, undef, undef),
647651
# Timestamp (seconds since epoch) indicating the earliest time the next update is
648652
# permitted.
@@ -658,12 +662,10 @@ our %variables = (
658662
# TODO: Create a timestamp type and change this to that type.
659663
'atime' => setv(T_NUMBER,0, 1, 0, undef),
660664
# Disposition of the most recent (or currently in progress) attempt to update the DDNS
661-
# service with the IP address in `wantip`. Anything other than `good`, including undef, is
662-
# treated as a failure.
663-
'status' => setv(T_ANY, 0, 1, undef, undef),
664-
# As `status`, but with `wantipv4`.
665+
# service with the IP address in `wantipv4` (or `wantip`, if an IPv4 address). Anything
666+
# other than `good`, including undef, is treated as a failure.
665667
'status-ipv4' => setv(T_ANY, 0, 1, undef, undef),
666-
# As `status`, but with `wantipv6`.
668+
# As `status-ipv4`, but with `wantipv6` (or `wantip`, if an IPv6 address).
667669
'status-ipv6' => setv(T_ANY, 0, 1, undef, undef),
668670
# Timestamp (seconds since epoch) of the most recent attempt that would have been made had
669671
# `min-interval` not inhibited the attempt. This is reset to 0 once an attempt is actually
@@ -1440,15 +1442,49 @@ sub update_nics {
14401442
delete($config{$h}{$_}) for qw(wantip wantipv4 wantipv6);
14411443
}
14421444

1443-
# Backwards compatibility:
1444-
# The legacy '--use' parameter sets 'wantip' and the legacy providers process this and
1445-
# set 'ip', 'status' accordingly.
1446-
# The new '--usev*' parameters set 'wantipv*' and the new providers set 'ipv*' and 'status-ipv*'.
1447-
# To allow gradual transition, we make sure both the old 'status' and 'ip' are being set
1448-
# accordingly to what new providers returned in the new 'status-ipv*' and 'ipv*' fields respectively.
1445+
# Backwards compatibility: Legacy protocol implementations read `wantip` and set `ip`
1446+
# and `status`. Modern protocol implementations read `wantipv4` and `wantipv6` and set
1447+
# `ipv4`, `ipv6`, `status-ipv4`, and `status-ipv6`. Make legacy implementations look
1448+
# like modern implementations by moving `ip` and `status` to the modern
1449+
# version-specific equivalents.
14491450
for my $h (@hosts) {
1450-
$config{$h}{'status'} //= $config{$h}{'status-ipv4'} // $config{$h}{'status-ipv6'};
1451-
$config{$h}{'ip'} //= $config{$h}{'ipv4'} // $config{$h}{'ipv6'};
1451+
local $_l = pushlogctx($h);
1452+
my $status = delete($config{$h}{'status'}) || next;
1453+
my $ip = $config{$h}{'ip'};
1454+
my $ipv = is_ipv4($ip) ? '4' : is_ipv6($ip) ? '6' : undef;
1455+
if (!defined($ipv)) {
1456+
warning("ddclient bug: legacy protocol set 'status' but did not set 'ip' " .
1457+
"to an IPv4 or IPv6 address: " . ($ip // '<undefined>'));
1458+
next;
1459+
}
1460+
# TODO: Currently $config{$h}{'ip'} is used for two distinct purposes: it holds the
1461+
# value of the --ip option, and it is updated by legacy protocols to hold the new
1462+
# IP address after an update. Fortunately, the --ip option is not used very often,
1463+
# and if it is, the values for the two use cases are usually (but not always) the
1464+
# same. This boolean is an imperfect attempt to identify whether 'ip' is being
1465+
# used for the --ip option, to avoid breaking some user's configuration. Protocols
1466+
# should be updated to not set $config{$h}{'ip'} because %config is for
1467+
# configuration, not update status (same goes for 'status', 'mtime', etc.).
1468+
my $ip_option = opt('use', $h) eq 'ip' || opt('usev6', $h) eq 'ip';
1469+
delete($config{$h}{'ip'}) if !$ip_option;
1470+
debug("legacy protocol; moving status to status-ipv$ipv and ip to ipv$ipv");
1471+
if (defined(my $vstatus = $config{$h}{"status-ipv$ipv"})) {
1472+
warning("ddclient bug: legacy protocol set both 'status' (to '$status') " .
1473+
"and 'status-ipv$ipv' (to '$vstatus')");
1474+
} else {
1475+
$config{$h}{"status-ipv$ipv"} = $status;
1476+
}
1477+
if (defined(my $vip = $config{$h}{"ipv$ipv"})) {
1478+
# TODO: See above comment for $ip_option. This is the same situation, but for
1479+
# 'ipv4' and 'ipv6'.
1480+
my $vip_option = opt("usev$ipv", $h) eq "ipv$ipv";
1481+
debug("unable to update 'ipv$ipv' to '$ip' " .
1482+
"because it is already set to '$vip'") if $vip_option;
1483+
warning("ddclient bug: legacy protocol set both 'ip' (to '$ip') " .
1484+
"and 'ipv$ipv' (to '$vip')") if !$vip_option;
1485+
} else {
1486+
$config{$h}{"ipv$ipv"} = $ip;
1487+
}
14521488
}
14531489

14541490
runpostscript(join ' ', keys %ipsv4, keys %ipsv6);
@@ -1510,7 +1546,7 @@ sub write_recap {
15101546
$recap{$h}{$v} = opt($v, $h);
15111547
}
15121548
} else {
1513-
for my $v (qw(atime wtime status status-ipv4 status-ipv6)) {
1549+
for my $v (qw(atime wtime status-ipv4 status-ipv6)) {
15141550
$recap{$h}{$v} = opt($v, $h);
15151551
}
15161552
}
@@ -1572,7 +1608,7 @@ sub read_recap {
15721608
next if !exists($config->{$h});
15731609
# TODO: Why is this limited to this set of variables? Why not copy every recap var
15741610
# defined for the host's protocol?
1575-
for (qw(atime mtime wtime ip ipv4 ipv6 status status-ipv4 status-ipv6)) {
1611+
for (qw(atime mtime wtime ip ipv4 ipv6 status-ipv4 status-ipv6)) {
15761612
# TODO: Isn't $config equal to \%recap here? If so, this is a no-op. What was the
15771613
# original intention behind this? To copy %recap values into %config? If so, is
15781614
# it better to just delete this and live with the current behavior (which doesn't
@@ -3407,7 +3443,6 @@ sub nic_updateable {
34073443
my ($host) = @_;
34083444
my $force_update = $protocols{opt('protocol', $host)}{force_update};
34093445
my $update = 0;
3410-
my $ip = $config{$host}{'wantip'};
34113446
my $ipv4 = $config{$host}{'wantipv4'};
34123447
my $ipv6 = $config{$host}{'wantipv6'};
34133448
my $inv_ip_warn_count = opt('max-warn');
@@ -3419,7 +3454,6 @@ sub nic_updateable {
34193454
my %prettyi = map({ ($_ => prettyinterval(opt($_, $host))); }
34203455
qw(max-interval min-error-interval min-interval));
34213456

3422-
$warned_ip{$host} = 0 if defined($ip);
34233457
$warned_ipv4{$host} = 0 if defined($ipv4);
34243458
$warned_ipv6{$host} = 0 if defined($ipv6);
34253459

@@ -3432,42 +3466,13 @@ sub nic_updateable {
34323466
$update = 1;
34333467

34343468
} elsif ($recap{$host}{'wtime'} && $recap{$host}{'wtime'} > $now) {
3435-
warning("cannot update IP from $previp to $ip until after $prettyt{'wtime'}");
3469+
warning("cannot update IP until after $prettyt{'wtime'}");
34363470

34373471
} elsif ($recap{$host}{'mtime'} && interval_expired($host, 'mtime', 'max-interval')) {
34383472
info("update forced because it has been $prettyi{'max-interval'} since the previous update (on $prettyt{'mtime'})");
34393473
$update = 1;
34403474

3441-
} elsif (defined($ip) && $previp ne $ip) {
3442-
## Check whether to update IP address for the "--use" method"
3443-
if (($recap{$host}{'status'} // '') eq 'good' &&
3444-
!interval_expired($host, 'mtime', 'min-interval')) {
3445-
warning("skipped update from $previp to $ip because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})")
3446-
if opt('verbose') || !($recap{$host}{'warned-min-interval'} // 0);
3447-
3448-
$recap{$host}{'warned-min-interval'} = $now;
3449-
3450-
} elsif (($recap{$host}{'status'} // '') ne 'good' &&
3451-
!interval_expired($host, 'atime', 'min-error-interval')) {
3452-
3453-
if (opt('verbose') || (!$recap{$host}{'warned-min-error-interval'} &&
3454-
($warned_ip{$host} // 0) < $inv_ip_warn_count)) {
3455-
warning("skipped update from $previp to $ip because it has been less than $prettyi{'min-error-interval'} since the previous update attempt (on $prettyt{'atime'}), which failed");
3456-
if (!$ip && !opt('verbose')) {
3457-
$warned_ip{$host} = ($warned_ip{$host} // 0) + 1;
3458-
warning("IP address undefined. Warned $inv_ip_warn_count times, suppressing further warnings")
3459-
if ($warned_ip{$host} >= $inv_ip_warn_count);
3460-
}
3461-
}
3462-
3463-
$recap{$host}{'warned-min-error-interval'} = $now;
3464-
3465-
} else {
3466-
$update = 1;
3467-
}
3468-
34693475
} elsif (defined($ipv4) && $previpv4 ne $ipv4) {
3470-
## Check whether to update IPv4 address for the "--usev4" method"
34713476
if (($recap{$host}{'status-ipv4'} // '') eq 'good' &&
34723477
!interval_expired($host, 'mtime', 'min-interval')) {
34733478
warning("skipped update from $previpv4 to $ipv4 because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})")
@@ -3495,7 +3500,6 @@ sub nic_updateable {
34953500
}
34963501

34973502
} elsif (defined($ipv6) && $previpv6 ne $ipv6) {
3498-
## Check whether to update IPv6 address for the "--usev6" method"
34993503
if (($recap{$host}{'status-ipv6'} // '') eq 'good' &&
35003504
!interval_expired($host, 'mtime', 'min-interval')) {
35013505
warning("skipped update from $previpv6 to $ipv6 because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})")
@@ -3532,15 +3536,15 @@ sub nic_updateable {
35323536

35333537
} else {
35343538
if (opt('verbose')) {
3535-
success("skipped update because IP address is already set to $ip")
3536-
if defined($ip);
35373539
success("skipped update because IPv4 address is already set to $ipv4")
35383540
if defined($ipv4);
35393541
success("skipped update because IPv6 address is already set to $ipv6")
35403542
if defined($ipv6);
35413543
}
35423544
}
35433545

3546+
# TODO: `status` is set by legacy protocol implementations. Remove it from this list once all
3547+
# legacy protocol implementations have been upgraded.
35443548
delete($config{$host}{$_}) for qw(status status-ipv4 status-ipv6 update);
35453549
if ($update) {
35463550
$config{$host}{'update'} = 1;
@@ -3549,7 +3553,7 @@ sub nic_updateable {
35493553
delete $recap{$host}{'warned-min-interval'};
35503554
delete $recap{$host}{'warned-min-error-interval'};
35513555
} else {
3552-
for (qw(status status-ipv4 status-ipv6)) {
3556+
for (qw(status-ipv4 status-ipv6)) {
35533557
$config{$host}{$_} = $recap{$host}{$_} if defined($recap{$host}{$_});
35543558
}
35553559
delete($config{$host}{$_}) for qw(wantip wantipv4 wantipv6);

0 commit comments

Comments
 (0)