From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/8930 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs() Date: Mon, 23 Nov 2015 12:01:55 -0500 Message-ID: <20151123170154.GI3818@brightrain.aerifal.cx> References: <1447756429-2304-2-git-send-email-jow@openwrt.org> <1447965790-8151-1-git-send-email-jow@openwrt.org> <20151121111446.6d23921c@vostro> <20151121192035.GC3818@brightrain.aerifal.cx> <20151123111249.56da0630@vostro> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1448298159 12897 80.91.229.3 (23 Nov 2015 17:02:39 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 23 Nov 2015 17:02:39 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-8943-gllmg-musl=m.gmane.org@lists.openwall.com Mon Nov 23 18:02:23 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1a0uVI-00062g-Ds for gllmg-musl@m.gmane.org; Mon, 23 Nov 2015 18:02:12 +0100 Original-Received: (qmail 9229 invoked by uid 550); 23 Nov 2015 17:02:10 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 8179 invoked from network); 23 Nov 2015 17:02:09 -0000 Content-Disposition: inline In-Reply-To: <20151123111249.56da0630@vostro> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:8930 Archived-At: On Mon, Nov 23, 2015 at 11:12:49AM +0200, Timo Teras wrote: > On Sat, 21 Nov 2015 14:20:35 -0500 > Rich Felker wrote: > > > On Sat, Nov 21, 2015 at 11:14:46AM +0200, Timo Teras wrote: > > > On Thu, 19 Nov 2015 21:43:10 +0100 > > > Jo-Philipp Wich wrote: > > > > > > > With point-to-point interfaces, the IFA_ADDRESS netlink attribute > > > > contains the peer address while an extra attribute IFA_LOCAL > > > > carries the actual local interface address. > > > > > > > > Both the glibc and uclibc implementations of getifaddrs() handle > > > > this case by moving the ifa_addr contents to the broadcast/remote > > > > address union and overwriting ifa_addr upon receipt of an > > > > IFA_LOCAL attribute. > > > > > > > > This patch adds the same special treatment logic of IFA_LOCAL to > > > > musl's implementation of getifaddrs() in order to align its > > > > behaviour with that of uclibc and musl. > > > > > > > > Signed-off-by: Jo-Philipp Wich > > > > --- > > > > Changelog v2: > > > > * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order > > > > * Remove misleading comment for IFA_BROADCAST, no such attribute > > > > on ptp links --- > > > > src/network/getifaddrs.c | 19 ++++++++++++++++--- > > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > I wrote the code before looking into how ptp links are reported, and > > > just assumed it'd be somehow consistent. But IFA_ADDRESS indeed is > > > the peer for ptp links. How nicely inconsistent from kernel side ;) > > > > > > Seems iproute2 basically does: > > > 1. If IFA_LOCAL not set, copy IFA_ADDRESS to it > > > 2. If IFA_ADDRESS is not set, copy IFA_LOCAL to it > > > 3. Print IFA_LOCAL as local address > > > 4. Print IFA_ADDRESS as peer address if it's not equal to IFA_LOCAL > > > > > > So this looks right to me. > > > > Are you sure? The new patch seems to have exactly the same issue with > > depending on the order of the records as the old patch had. To solve > > it I think both need to be stored in temp storage during the loop, > > then code after the loop has to resolve which to use. Am I missing > > something? > > v2 patch in pseudo-code does: > IFA_ADDRESS: > if ifa_addr set earlier > ifa_dstaddr = this > else > ifa_addr = this > > IFA_LOCAL: > if ifa_addr set earlier > ifa_dstaddr = ifa_addr > ifa_addr = this > > so it does look right to me, and handles whatever order they are in: > > IFA_ADDRESS then IFA_LOCAL: > IFA_ADDRESS sets ifa_addr > IFA_LOCAL moves ifa_addr to ifa_dstaddr and sets ifa_addr > > IFA_LOCAL then IFA_ADDRESS: > IFA_LOCAL sets ifa_addr > IFA_ADDRESS sets ifa_dstaddr > > The only side affect might be if you get two IFA_LOCAL addresses, the > first goes to ifa_dstaddr. But that's invalid input, kernel does not > create it, so I think we need to care about it. Thanks for explaining it. I think you're right. > It might be more obvious what is going on if we store the RTA info for > IFA_ADDRESS/IFA_LOCAL and do the logic after the loop. But functionally > it should be the same. Yes, this would be more clear and was the fix I expected, but I'm okay with this version as long as it's correct. Any further comments before I apply this? Rich