From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH v6 1/1] ddns: Add provider Feste-IP.Net
Date: Thu, 20 May 2021 11:00:26 +0100
Message-ID: <EFBC9CFD-B476-4037-8B6F-5A72E0CE8B83@ipfire.org>
In-Reply-To: <54e378f2-ad73-9395-88ea-9436008fc6b7@posteo.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6359056743365130186=="
List-Id: <development.lists.ipfire.org>

--===============6359056743365130186==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello Martin,

Thanks for working on ddns, but *please* read Peter=E2=80=99s email.

This is now the third patch (I don=E2=80=99t know why you are at number 6 alr=
eady) and the idea of the review process is to submit something that at least=
 works. You can ask all sorts of questions about anything you need to know, b=
ut you will have to listen to what people answer.

This patch is fundamentally broken. I will comment on that further below.

Secondly, we cannot even think about merging something that isn=E2=80=99t tes=
ted at all.

So I would urge you to look at any of the other providers, because what you a=
re trying to implement has already been implemented here for example:

  https://git.ipfire.org/?p=3Dddns.git;a=3Dblob;f=3Dsrc/ddns/providers.py;h=
=3D56e6620c78ab3d9de1c945147f86e70a8d8614d7;hb=3DHEAD#l975

@Stefan: We might want to have a class variable in the DynDNS protocol that a=
dds the IPv6 address to a field that is named in that variable. That way, we =
avoid any code duplication.

> On 18 May 2021, at 19:49, Martin Krieger <makrie(a)posteo.de> wrote:
>=20
> From 37fb0237932fb12bdd635e9cb5e01b0bf9f03dda Mon Sep 17 00:00:00 2001
> From: Martin Krieger <makrie(a)posteo.de>
> Date: Tue, 18 May 2021 19:42:44 +0200
> Subject: [PATCH v6 1/1] ddns: Add provider Feste-IP.Net
>=20
> Comments:
> Provider supports IPv4, IPv6 & DS (Dual-Stack)
>=20
> Changelog:
>=20
> 18.05.2021
> Improved failure handling.
> IPv6 & DS still not checked, because my network connection is pure IPv4.
>=20
> 17.05.2021
> 5th attempt ([PATCH v5 1/1])
>=20
> 16.05.2021
> 4th attempt ([PATCH v4 1/1])
>=20
> 08.05.2021
> 3rd attempt ([PATCH v3 1/1])
>=20
> 06.05.2021
> 2nd attempt ([PATCH v2 1/1])
>=20
> 05.05.2021
> 1st attempt ([PATCH v1 1/1])
>=20
> Signed-off-by: Martin Krieger <makrie(a)posteo.de>..
> ---
> README                |  1 +
> src/ddns/providers.py | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 32 insertions(+)
>=20
> diff --git a/README b/README
> index b6decb35c338..fa6ce5e598b8 100644
> --- a/README
> +++ b/README
> @@ -68,6 +68,7 @@ SUPPORTED PROVIDERS:
> 	easydns.com
> 	enom.com
> 	entrydns.net
> +	feste-ip.net
> 	freedns.afraid.org
> 	inwx.com|de|at|ch|es
> 	itsdns.de
> diff --git a/src/ddns/providers.py b/src/ddns/providers.py
> index 56e6620c78ab..b7e14482a0b1 100644
> --- a/src/ddns/providers.py
> +++ b/src/ddns/providers.py
> @@ -1179,6 +1179,37 @@ class DDNSProviderEntryDNS(DDNSProvider):
> 		raise DDNSUpdateError
>=20
>=20
> +class DDNSProviderFesteIPNet(DDNSProtocolDynDNS2, DDNSProvider):
> +        handle    =3D "feste-ip.net"
> +        name      =3D "Feste-IP.Net"
> +        website   =3D "https://www.feste-ip.net/"
> +
> +        # Information about the format of the request is to be found
> +        # https://forum.feste-ip.net/viewtopic.php?f=3D13&t=3D469
> +
> +        myips =3D ("myip","myip2")
> +
> +        url =3D "https://members.feste-ip.net/nic/update/"
> +
> +        def update(self):
> +                data =3D {
> +                        "hostname" : self.hostname
> +                }
> +
> +                for proto in DDNSProvider.protocols:
> +                        idx =3D 0
> +                        tmpip =3D self.get_address(proto)
> +                        if tmpip:
> +                                data[self.myips[idx]] =3D tmpip
> +                                idx +=3D 1

Setting idx is useless here. You will reset to zero in every iteration of the=
 loop. Since it is being incremented last and then reset to zero, idx will *a=
lways* be zero.

> +
> +                if self.myips[0] in data:
> +                        self.send_request(data)
> +                        return#

You will only send one request here and only if it contains an IPv4 address. =
Why? What happens to systems that only have IPv6?

-Michael

> +
> +                raise DDNSUpdateError
> +
> +
> class DDNSProviderFreeDNSAfraidOrg(DDNSProvider):
> 	handle    =3D "freedns.afraid.org"
> 	name      =3D "freedns.afraid.org"
> --=20
> 2.31.0
>=20
>=20
>=20


--===============6359056743365130186==--