From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter =?utf-8?q?M=C3=BCller?= <peter.mueller@ipfire.org>
To: location@lists.ipfire.org
Subject: Re: [PATCH 1/2] location-importer: Introduce auxiliary function to
 sanitise ASNs
Date: Wed, 13 Oct 2021 18:33:53 +0200
Message-ID: <cc204412-81eb-13ea-8adc-76c34268cfa5@ipfire.org>
In-Reply-To: <E8561289-9FA9-4090-941E-CFB89D540FF2@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0388207595119721559=="
List-Id: <location.lists.ipfire.org>

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

Hello Michael,

thanks for your reply.

> Hello,
>=20
>> On 10 Oct 2021, at 17:16, Peter M=C3=BCller <peter.mueller(a)ipfire.org> w=
rote:
>>
>> Signed-off-by: Peter M=C3=BCller <peter.mueller(a)ipfire.org>
>> ---
>> src/python/location-importer.in | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/python/location-importer.in b/src/python/location-importe=
r.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
>>
>> +	def _check_parsed_asn(self, asn):
>> +		"""
>> +			Assistive function to filter Autonomous System Numbers not being suita=
ble
>> +			for adding to our database. Returns False in such cases, and True othe=
rwise.
>> +		"""
>> +
>> +		if not asn or not isinstance(asn, int):
>> +			return False
>=20
> Does this happen that a non-integer is being passed to this function?

What's wrong with input validation? I _like_ input validation. :-)

Seriously: Anything else than an integer does not make sense for an ASN. Sure=
, this
function is not intended to get anything else, but we will never know. Better=
 to be
safe than sorry.

> You also return False for zero without logging the message.

True. Since there will probably a second version of this patchset, I will ens=
ure it
logs anything useful in this case.

> I would suggest to drop the check above.

Frankly, I don't see why.

>> +
>> +		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 th=
en iterate over that until you find a match.

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...

>=20
>> +
>> +		# ASN is fine if we made it here...
>> +		return True
>=20
> Ellipses in comments are sometimes weird...

???

Thanks, and best regards,
Peter M=C3=BCller

>=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
>>
>> -				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
>>
>> 				# Skip any AS name that appears to be a placeholder for a different RI=
R or entity...
>> --=20
>> 2.26.2
>=20

--===============0388207595119721559==--