Change in vdsm[master]: netinfo: get an interface using one of IP addresses assgined...

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Wed Jun 5 06:44:41 UTC 2013


Zhou Zheng Sheng has posted comments on this change.

Change subject: netinfo: get an interface using one of IP addresses assgined to it
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(3 inline comments)

Basically I'm fine with the patch. -1 to have my questions answered, then I'll clear the -1 flag.

....................................................
File lib/vdsm/netinfo.py
Line 481:     """Return an IPv6 IPv4-mapped address for the IPv4 address"""
Line 482:     mapped = None
Line 483: 
Line 484:     try:
Line 485:         ipv4num = struct.unpack('>I', socket.inet_pton(socket.AF_INET, ip))[0]
Firstly, is '!I' better than '>I' ?

Secondly, can this function be simplified to the following?

 ipBin = '\x00' * 10 + '\xff\xff' + socket.inet_pton(socket.AF_INET, ip)
 mapped = socket.inet_ntop(socket.AF_INET6, ipBin)
Line 486:         ipv6num = ipv4num | (0xffff << 32)
Line 487:         ipv6struct = struct.pack('>QQ', *divmod(ipv6num, 2 ** 64))
Line 488:         mapped = socket.inet_ntop(socket.AF_INET6, ipv6struct)
Line 489:     except socket.error as e:


Line 500:         # also match a IPv4-mapped IPv6 address
Line 501:         ipaddrs.append(IPv4toMapped(ipaddrs[-1]))
Line 502:         if ip in ipaddrs:
Line 503:             return dev.device
Line 504:     return ''
The presentation string of the same IPv6 address can be various, but the binary data is the same. Though the address strings are produced by ethtool and getsockname, and are in a good form, I still think comparing IP address in their binary format is better generally.

The IPv6 and IPv4 addresses can be converted by inet_pton then put in ipaddrs. The IPv4 mapped IPv6 address binary can be produced by the following.

 ip6Bin = '\x00' * 10 + '\xff\xff' + socket.inet_pton(socket.AF_INET, '127.0.0.1')

Then append this to ipaddrs.

The input argument "ip" of getIfaceByIP function can be converted to binary format as well.

 family = socket.getaddrinfo(ip, 0, 0, 0, 0, socket.AI_NUMERICHOST)[0][0]
 ipBin = socket.inet_pton(family, ip)

Then

 if ipBin in ipaddrs:
     ...

should also work.

In C programming, the IP address structure sockaddr_storage contains a ss_family field, it's very convenient to manipulate the address data with the help of ss_family. In Python it always needs to determine the family by getaddrinfo before invoking inet_pton. I think it's not a good design for Python socket API accept IP address in presentation format, it forces us store the address string but not binary.

 
 
It's OK if you'd like using address strings. I am not very strongly against it. Could you consider simplifying the IPv4toMapped function? I find it a bit hard to follow, so I post a simplified version in the related comment above. What do you think of it?
Line 505: 
Line 506: 
Line 507: class NetInfo(object):
Line 508:     def __init__(self, _netinfo=None):


....................................................
File tests/capsTests.py
Line 93
Line 94
Line 95
Line 96
Line 97
We can test your new getIfaceByIP implementation as well.

 self.assertEqual('lo', getIfaceByIP('127.0.0.1'))
 self.assertEqual('lo', getIfaceByIP('::1'))

We can also test IPv4toMapped implementation.

 self.assertEqual('::ffff:127.0.0.1', IPv4toMapped('127.0.0.1'))


--
To view, visit http://gerrit.ovirt.org/11519
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I239f92c527524e5651f51712841c3eafcf659e53
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Hunt Xu <mhuntxu at gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Hunt Xu <mhuntxu at gmail.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list