From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: location@lists.ipfire.org Subject: Re: [PATCH 1/2] location-importer: Introduce auxiliary function to sanitise ASNs Date: Thu, 14 Oct 2021 19:19:26 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6069478280611538503==" List-Id: --===============6069478280611538503== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, > On 13 Oct 2021, at 17:33, Peter M=C3=BCller wr= ote: >=20 > Hello Michael, >=20 > thanks for your reply. >=20 >> Hello, >>=20 >>> On 10 Oct 2021, at 17:16, Peter M=C3=BCller = wrote: >>>=20 >>> Signed-off-by: Peter M=C3=BCller >>> --- >>> src/python/location-importer.in | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>>=20 >>> diff --git a/src/python/location-importer.in b/src/python/location-import= er.in >>> index da058d3..c2b3e41 100644 >>> --- a/src/python/location-importer.in >>> +++ b/src/python/location-importer.in >>> @@ -574,6 +574,22 @@ class CLI(object): >>> # be suitable for libloc consumption... >>> return True >>>=20 >>> + def _check_parsed_asn(self, asn): >>> + """ >>> + Assistive function to filter Autonomous System Numbers not being suit= able >>> + for adding to our database. Returns False in such cases, and True oth= erwise. >>> + """ >>> + >>> + if not asn or not isinstance(asn, int): >>> + return False >>=20 >> Does this happen that a non-integer is being passed to this function? >=20 > What's wrong with input validation? I _like_ input validation. :-) There is nothing wrong with that. You are just checking the developer here an= d I am not sure whether you want that or not. > Seriously: Anything else than an integer does not make sense for an ASN. Su= re, this > function is not intended to get anything else, but we will never know. Bett= er to be > safe than sorry. Not entirely. You want code to perform. If you want to be 100% use, Python is= n=E2=80=99t the language this parser should be written in. Nothing else but an integer makes sense. The question is how do you want to t= reat zero? >> You also return False for zero without logging the message. >=20 > True. Since there will probably a second version of this patchset, I will e= nsure it > logs anything useful in this case. >=20 >> I would suggest to drop the check above. >=20 > Frankly, I don't see why. >=20 >>> + >>> + if not ((1 <=3D asn and asn <=3D 23455) or (23457 <=3D asn and asn <= =3D 64495) or (131072 <=3D asn and asn <=3D 4199999999)): >>> + log.debug("Skipping invalid ASN: %s" % asn) >>> + return False >>=20 >> This works, but I do not consider this very Pythonic. >>=20 >> I would have written a tuple which conatins one tuple for each range and t= hen iterate over that until you find a match. >=20 > Far from being a Python developer, this wouldn't have come to my mind. But = if it's > Pythonic, I'll do so. When in Rome... I don=E2=80=99t make the rules. That is just how I would do it: * Data in one place * A short algorithm that works on the data In C I would hope that the compiler makes it fast. >=20 >>=20 >>> + >>> + # ASN is fine if we made it here... >>> + return True >>=20 >> Ellipses in comments are sometimes weird... >=20 > ??? This one left the comment kind of open ended. Making it sound kind of unlikel= y. >=20 > Thanks, and best regards, > Peter M=C3=BCller >=20 >>=20 >>> + >>> def _parse_block(self, block, source_key, validcountries =3D None): >>> # Get first line to find out what type of block this is >>> line =3D block[0] >>> @@ -829,8 +845,8 @@ class CLI(object): >>> log.debug("Skipping ARIN AS names line not containing an integer for= ASN") >>> continue >>>=20 >>> - if not ((1 <=3D asn and asn <=3D 23455) or (23457 <=3D asn and asn <= =3D 64495) or (131072 <=3D asn and asn <=3D 4199999999)): >>> - log.debug("Skipping ARIN AS names line not containing a valid ASN: = %s" % asn) >>> + # Filter invalid ASNs... >>> + if not self._check_parsed_asn(asn): >>> continue >>>=20 >>> # Skip any AS name that appears to be a placeholder for a different R= IR or entity... >>> --=20 >>> 2.26.2 --===============6069478280611538503==--