> @@ -509,8 +510,11 @@ def isWireless(dev):
> else:
> return False
>
> -# Get the IP address for a network device.
> -def getIPAddress(dev):
> +# Get IP addresses for a network device.
> +# Returns list of ipv4 and ipv6 addresses.
> +# With version=4 returns only ipv4 addresses,
> +# with version=6 returns only ipv6 addresses.
> +def getIPAddresses(dev, version=None):
> if dev == ' or dev is None:
> return None
>

What I was thinking here is that version should default to 4, since
that's the thing we'll most typically want and having an essentially
required parameter default to None doesn't make any sense to me.

> + if not version == 6:
> + ip4_config_path = device_props_iface.Get(NM_DEVICE_IFACE, 'Ip4Config')
> + if ip4_config_path != '/':
> + ip4_config_obj = bus.get_object(NM_SERVICE, ip4_config_path)
> + ip4_config_props = dbus.Interface(ip4_config_obj, DBUS_PROPS_IFACE)
> +
> + # addresses (3-element list: ipaddr, netmask, gateway)
> + addrs = ip4_config_props.Get(NM_IP4CONFIG_IFACE, "Addresses")
> + for addr in addrs:
> + try:
> + tmp = struct.pack('I', addr[0])
> + ipaddr = socket.inet_ntop(socket.AF_INET, tmp)
> + addresses.append(ipaddr)
> + except ValueError as e:
> + log.debug("Exception caught trying to convert IP address %s: %s" %
> + (addr, e))
> +
> + if not version == 4:
> + ip6_config_path = device_props_iface.Get(NM_DEVICE_IFACE, 'Ip6Config')
> + if ip6_config_path != '/':
> + ip6_config_obj = bus.get_object(NM_SERVICE, ip6_config_path)
> + ip6_config_props = dbus.Interface(ip6_config_obj, DBUS_PROPS_IFACE)
> +
> + addrs = ip6_config_props.Get(NM_IP6CONFIG_IFACE, "Addresses")
> + for addr in addrs:
> + try:
> + addrstr = "".join(str(byte) for byte in addr[0])
> + ipaddr = socket.inet_ntop(socket.AF_INET6, addrstr)
> + # XXX - should we prefer Global or Site-Local types?
> + # does NM prefer them?
> + addresses.append(ipaddr)
> + except ValueError as e:
> + log.debug("Exception caught trying to convert IP address %s: %s" %
> + (addr, e))
> +
> + return addresses

I find the negation thing here unnecessarily confusing. So if I pass
version=47 to getIPAddresses, I'll get everything. If I pass
version=True, I'll get everything. Same for version="cornbread" and
version=("left", "right").

Just converting this to "if version == 4:" and "if version == 6:" means
you'll get exactly what you want should you pass the correct parameter.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/29/2010 01:05 PM, Chris Lumens wrote:
>> @@ -509,8 +510,11 @@ def isWireless(dev):
>> else:
>> return False
>>
>> -# Get the IP address for a network device.
>> -def getIPAddress(dev):
>> +# Get IP addresses for a network device.
>> +# Returns list of ipv4 and ipv6 addresses.
>> +# With version=4 returns only ipv4 addresses,
>> +# with version=6 returns only ipv6 addresses.
>> +def getIPAddresses(dev, version=None):
>> if dev == ' or dev is None:
>> return None
>>

Could we put comments like these into docstrings instead of comments?
Just in case I want to run doxygen on the code

- --
Brian C. Lane <bcl@redhat.com>
Red Hat / Port Orchard, WA
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEVAwUBTCuCHBF+jBaO/jp/AQLEQgf+I26ra3krODsTVvD/nn3pWS0phF3Vd1y4
tgxMqDaC+zgC24lewCUUkQrkdh2pZZNEhCt996aoobjSONes/YE4LnLGGbJV9zkF
GHAZFiuvUXhfDuCIbATn4hRm+j/zyhJnd8UVUo36Ase9j92uwEi64hj5RTcqe2KB
gNfmSm/B4VZKGVmo+rcDTQLU8Z5mqqIr3XraVN23msSa2k8JrTQRlwAwa fsbeouL
6DoJn8lWszQbqL/EVxzh3NsoBQ7th0PXXYIJ3jWeJ5FFCUoOdYO7bpteEtFHfYf7
vEB87TJ2bN6P4qmeHiF51nuDQ1+oUxe7ZLbJc5yNxXvLQ2gkqD +orw==
=VQqa
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On 06/30/2010 07:42 PM, Brian C. Lane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/29/2010 01:05 PM, Chris Lumens wrote:


@@ -509,8 +510,11 @@ def isWireless(dev):
else:
return False

-# Get the IP address for a network device.
-def getIPAddress(dev):
+# Get IP addresses for a network device.
+# Returns list of ipv4 and ipv6 addresses.
+# With version=4 returns only ipv4 addresses,
+# with version=6 returns only ipv6 addresses.
+def getIPAddresses(dev, version=None):
if dev == ' or dev is None:
return None



Could we put comments like these into docstrings instead of comments?
Just in case I want to run doxygen on the code




Yes, I had been thinking about doing so in my patches,
but here my inner calling to be consistent (apparently in
wrong aspect here) with existing code won again. Next time

Radek

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list