From mboxrd@z Thu Jan 1 00:00:00 1970 From: Serhey Popovich Subject: Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling Date: Tue, 19 Dec 2017 18:05:02 +0200 Message-ID: <1181c406-fb7f-d81b-5a45-1b170c1b64dd@gmail.com> References: <1513623248-7689-1-git-send-email-serhe.popovych@gmail.com> <1513623248-7689-2-git-send-email-serhe.popovych@gmail.com> <20171218112359.1ef8db23@xeon-e3> <7f83992a-90e0-4f15-2be4-7348a6742e6c@gmail.com> <20171218132231.6dcf7b54@xeon-e3> <18e98fc6-4145-0bb9-143d-d4305d22bdc8@gmail.com> <20171219075951.7aca0d53@xeon-e3> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="dMkSNxElEVcoIiH1U7ihoazEONGJrKFjA" Cc: netdev@vger.kernel.org To: Stephen Hemminger Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:44101 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751713AbdLSQFU (ORCPT ); Tue, 19 Dec 2017 11:05:20 -0500 Received: by mail-wm0-f67.google.com with SMTP id t8so4662537wmc.3 for ; Tue, 19 Dec 2017 08:05:19 -0800 (PST) In-Reply-To: <20171219075951.7aca0d53@xeon-e3> Sender: netdev-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --dMkSNxElEVcoIiH1U7ihoazEONGJrKFjA Content-Type: multipart/mixed; boundary="5mnst3n6r8CSVCqJAWYUfGtAd52clA66m"; protected-headers="v1" From: Serhey Popovich To: Stephen Hemminger Cc: netdev@vger.kernel.org Message-ID: <1181c406-fb7f-d81b-5a45-1b170c1b64dd@gmail.com> Subject: Re: [PATCH iproute2 1/3] iplink: Improve index parameter handling References: <1513623248-7689-1-git-send-email-serhe.popovych@gmail.com> <1513623248-7689-2-git-send-email-serhe.popovych@gmail.com> <20171218112359.1ef8db23@xeon-e3> <7f83992a-90e0-4f15-2be4-7348a6742e6c@gmail.com> <20171218132231.6dcf7b54@xeon-e3> <18e98fc6-4145-0bb9-143d-d4305d22bdc8@gmail.com> <20171219075951.7aca0d53@xeon-e3> In-Reply-To: <20171219075951.7aca0d53@xeon-e3> --5mnst3n6r8CSVCqJAWYUfGtAd52clA66m Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Stephen Hemminger wrote: > On Mon, 18 Dec 2017 23:37:09 +0200 > Serhey Popovich wrote: >=20 >> Stephen Hemminger wrote: >>> On Mon, 18 Dec 2017 23:02:07 +0200 >>> Serhey Popovich wrote: >>> =20 >>>> Stephen Hemminger wrote: =20 >>>>> On Mon, 18 Dec 2017 20:54:06 +0200 >>>>> Serhey Popovych wrote: >>>>> =20 >>>>>> diff --git a/ip/iplink.c b/ip/iplink.c >>>>>> index 1e685cc..4f9c169 100644 >>>>>> --- a/ip/iplink.c >>>>>> +++ b/ip/iplink.c >>>>>> @@ -586,8 +586,10 @@ int iplink_parse(int argc, char **argv, struc= t iplink_req *req, >>>>>> *name =3D *argv; >>>>>> } else if (strcmp(*argv, "index") =3D=3D 0) { >>>>>> NEXT_ARG(); >>>>>> + if (*index) >>>>>> + duparg("index", *argv); >>>>>> *index =3D atoi(*argv); >>>>>> - if (*index < 0) >>>>>> + if (*index <=3D 0) =20 >>>>> >>>>> Why not use strtoul instead of atoi? =20 >>>> Do not see reason for strtoul() instead atoi(): >>>> >>>> 1) main arg: indexes in kernel represented as "int", which is >>>> signed. <=3D 0 values are reserved for various special purposes= >>>> (see net/core/fib_rules.c on how device matching implemented). >>>> >>>> Configuring network device manually with index <=3D 0 is not co= rrect >>>> (however possible). Kernel itself never chooses ifindex <=3D 0.= >>>> >>>> Having unsigned int > 0x7fffffff actually means index <=3D 0. >>>> >>>> 2) this is not single place in iproute2 where it is used: not >>>> going to remove last user. >>>> >>>> 3) make changes clear and transparent for review. =20 >>> >>> I would rather all of iproute2 correctly handles unsigned values. >>> Too much code is old K&R style C "the world is an int" and "who needs= >>> to check for negative". =20 >> >> You are right :(. I'm just trying to improve things a bit. >> >>> >>> There already is get_unsigned() in iproute2 util functions. =20 >> This is good one based on strtoul(). But do we want to submit say >> index =3D (unsigned int)2147483648(0x7fffffff) to the kernel that is >> illegal from it's perspective? >> >> Or do you mean I can prepare treewide change to replace atoi() with >> get_unsigned()/get_integer() where appropriate? >> >> We already check if (*index < 0) since commit 3c682146aeff >> (iplink: forbid negative ifindex and modifying ifindex), and I just >> put index =3D=3D 0 in the same range of invalid indexes. >> >=20 > The legacy BSD ABI for interfaces uses int, so that sets the upper > bound for kernel. >=20 > The netlink ABI limit is u32 for ifindex so technically 1..UINT32_MAX a= re > possible values but kernel is bound by BSD mistake. Thank you for in depth explanation! >=20 > I will take the original patch. >=20 >=20 --5mnst3n6r8CSVCqJAWYUfGtAd52clA66m-- --dMkSNxElEVcoIiH1U7ihoazEONGJrKFjA Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQEcBAEBAgAGBQJaOTi9AAoJEBTawMmQ61bBdIwH/3a/CWR8ISkdU0FnyhGocF6a uQv7GzpbTdIEXhZDgjWiV3rj4b0GyTzhbrEculjnPPcL9jxYpL3UZ98OfE2WBDC/ Tm4J27EMFUqe+fLrkFCIys4+3Lx3zyt/9j0uzD9zCW12MhoWgIAh4ewtJeQFmo3r VQEL8w2d6j67/80rKwUMcRMrsDnK0rDEexjlMwB5F7gjQXr13hSpfGG4H6bzY9r3 n9/A0Cp8R48GtY4ZI4qDUhRrZmIAmFOxBDaEe98DKFrIbFQdaNrudC5Sl13ZiMiC jNvBl7oHfpD6tkI2v2mipYN+fMGInfJcCJtTT9yVlmEkHWZcR1wqETQrd4iZBs0= =Iw6g -----END PGP SIGNATURE----- --dMkSNxElEVcoIiH1U7ihoazEONGJrKFjA--