Date: Mon, 23 Nov 2015 12:01:55 -0500 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs() On Mon, Nov 23, 2015 at 11:12:49AM +0200, Timo Teras wrote: > On Sat, 21 Nov 2015 14:20:35 -0500 > Rich Felker <dalias@...c.org> 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 <jow@...nwrt.org> 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 <jow@...nwrt.org> > > > > --- > > > > 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.