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