[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.
