Date: Mon, 23 Nov 2015 11:12:49 +0200 From: Timo Teras <timo.teras@....fi> To: Rich Felker <dalias@...c.org> Cc: musl@...ts.openwall.com Subject: Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs() 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. 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. Or am I missing something?
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.