[Python-checkins] cpython: Issue #1746656: Add if_nameindex, if_nametoindex, if_indextoname

Mon May 16 05:14:25 CEST 2011

Hi,
On 15/05/2011 10.27, gregory.p.smith wrote:
> http://hg.python.org/cpython/rev/c2515cb23d9b
> changeset: 70136:c2515cb23d9b
> parent: 70134:94e3c44b0662
> user: Gregory P. Smith<greg at krypto.org>
> date: Sun May 15 00:26:45 2011 -0700
> summary:
> Issue #1746656: Add if_nameindex, if_nametoindex, if_indextoname
> methods to the socket module.
>> files:
> Doc/library/socket.rst | 33 +
> Lib/test/test_socket.py | 10 +
> Misc/NEWS | 3 +
> Modules/socketmodule.c | 110 +++++
> configure | 523 ++++++++++++++-------------
> configure.in | 1 +
> pyconfig.h.in | 3 +
> 7 files changed, 432 insertions(+), 251 deletions(-)
>>> diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst
> --- a/Doc/library/socket.rst
> +++ b/Doc/library/socket.rst
> @@ -536,6 +536,39 @@
> .. versionadded:: 3.3
>>> +.. function:: if_nameindex()
> +
> + Returns a list of network interface information
> + (index, name as a byte string) tuples.

This should be 'Return' (ditto for the next two functions).
I assume index is an int and 'as a byte string' refers only to the name, 
but saying something like "(index as an int, name as a byte string)" 
would make this clearer IMHO.
> + :exc:`socket.error` if the system call fails for any reason.

"Raise :exc:`socket.error` if ..." (ditto for the next two). The "for 
any reason" is probably unnecessary.
> +
> + Availability: Unix.
> +
> + .. versionadded:: 3.3
> +
> +
> +.. function:: if_nametoindex(if_name)
> +
> + Returns a network interface index number corresponding to an
> + interface name byte string.

I would say "an interface name as a byte string", otherwise this can be 
parsed as "the byte string of the interface name", "the name byte string 
of the interface" or even "the string of the interface name byte". On 
the other hand 'as a byte string' might also refer to the returned index 
so maybe something like this is better:
"Return the network interface index number corresponding to the 
interface name *if_name*. *if_name* must be a byte string."
> + :exc:`socket.error` if no interface with the given name exists.
> +
> + Availability: Unix.
> +
> + .. versionadded:: 3.3
> +
> +
> +.. function:: if_indextoname(if_index)
> +
> + Returns a network interface name byte string corresponding to a
> + interface index.
> + :exc:`socket.error` if no interface with the given index exists.
> +
> + Availability: Unix.
> +
> + .. versionadded:: 3.3
> +
> +
> .. data:: SocketType
>> This is a Python type object that represents the socket object type. It is the
> diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
> --- a/Lib/test/test_socket.py
> +++ b/Lib/test/test_socket.py
> @@ -372,6 +372,16 @@
> finally:
> socket.sethostname(oldhn)
>> + @unittest.skipUnless(hasattr(socket, 'if_nameindex'),
> + 'socket.if_nameindex() not available.')
> + def testInterfaceNameIndex(self):
> + interfaces = socket.if_nameindex()
> + for index, name in interfaces:
> + # interface indices are non-zero integers
> + self.assertGreater(index, 0)
> + self.assertEqual(index, socket.if_nametoindex(name))
> + self.assertEqual(name, socket.if_indextoname(index))
> +

A test for the error conditions should be added too.
> def testRefCountGetNameInfo(self):
> # Testing reference count for getnameinfo
> if hasattr(sys, "getrefcount"):
> diff --git a/Misc/NEWS b/Misc/NEWS
> --- a/Misc/NEWS
> +++ b/Misc/NEWS
> @@ -10,6 +10,9 @@
> Core and Builtins
> -----------------
>> +- Added the if_nameindex, if_indextoname, if_nametoindex methods to
> + the socket module as requested in issue #1746656.
> +

This should be in the form "- Issue #NNNN: description".
> - Issue #12044: Fixed subprocess.Popen when used as a context manager to
> wait for the process to end when exiting the context to avoid unintentionally
> leaving zombie processes around.
> diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
> --- a/Modules/socketmodule.c
> +++ b/Modules/socketmodule.c
> @@ -42,6 +42,9 @@
> - socket.inet_ntoa(packed IP) -> IP address string
> - socket.getdefaulttimeout() -> None | float
> - socket.setdefaulttimeout(None | float)
> +- socket.if_nameindex() -> list of tuples (if_index, if_name)
> +- socket.if_nametoindex(name) -> corresponding interface index
> +- socket.if_indextoname(index) -> corresponding interface name
> - an Internet socket address is a pair (hostname, port)
> where hostname can be anything recognized by gethostbyname()
> (including the dd.dd.dd.dd notation) and port is in host byte order
> @@ -133,6 +136,9 @@
> setsockopt(level, optname, value) -- set socket options\n\
> settimeout(None | float) -- set or clear the timeout\n\
> shutdown(how) -- shut down traffic in one or both directions\n\
> +if_nameindex() -- return all network interface indices and names\n\
> +if_nametoindex(name) -- returns the corresponding interface index\n\
> +if_indextoname(index) -- returns the corresponding interface name\n\

s/returns/return/g
> \n\
> [*] not available on all platforms!");
>> @@ -4267,6 +4273,102 @@
> When the socket module is first imported, the default is None.");
>>> +#ifdef HAVE_IF_NAMEINDEX
> +/* Python API for getting interface indices and names */
> +
> +static PyObject *
> +socket_if_nameindex(PyObject *self, PyObject *arg)
> +{
> + int i = 0;
> + PyObject *list;
> + struct if_nameindex *ni = if_nameindex();
> +
> + if (ni == NULL) {
> + PyErr_SetString(socket_error, "if_nameindex() returned NULL.");

Wouldn't something like "An error occurred in if_nameindex()." be better?
Returning NULL is just an implementation detail that people might not be 
aware of.
> + return NULL;
> + }
> +
> + list = PyList_New(0);
> + if (list == NULL) {
> + if_freenameindex(ni);
> + return NULL;
> + }
> +
> + while (ni[i].if_index != 0&& i< INT_MAX) {
> + PyObject *ni_tuple = Py_BuildValue(
> + "Iy", ni[i].if_index, ni[i].if_name);
> +
> + if (ni_tuple == NULL || PyList_Append(list, ni_tuple) == -1) {
> + Py_XDECREF(ni_tuple);
> + goto error;
> + }
> + Py_DECREF(ni_tuple);
> +
> + ++i;
> + }
> +
> + if_freenameindex(ni);
> + return list;
> +
> +error:
> + Py_DECREF(list);
> + if_freenameindex(ni);
> + return NULL;
> +}
> +
> +PyDoc_STRVAR(if_nameindex_doc,
> +"if_nameindex()\n\
> +\n\
> +Returns a list of network interface information (index, name) tuples.");

s/Returns/Return/
> +
> +
> +PyObject*
> +socket_if_nametoindex(PyObject *self, PyObject *arg)
> +{
> + char* ifname = PyBytes_AsString(arg);
> + unsigned long index;
> +
> + if (ifname == NULL)
> + return NULL;
> +
> + index = if_nametoindex(ifname);
> + if (index == 0) {
> + PyErr_SetString(socket_error, "no interface with this name");
> + return NULL;
> + }
> +
> + return PyLong_FromUnsignedLong(index);
> +}
> +
> +PyDoc_STRVAR(if_nametoindex_doc,
> +"if_nametoindex(if_name)\n\
> +\n\
> +Returns the interface index corresponding to the interface name if_name.");

Ditto.
> +
> +
> +PyObject*
> +socket_if_indextoname(PyObject *self, PyObject *arg)
> +{
> + unsigned long index = PyLong_AsUnsignedLongMask(arg);
> + char name[IF_NAMESIZE + 1]; /* or use IFNAMSIZ ?*/
> + char *ret = if_indextoname(index,&name[0]);
> +
> + if (ret == NULL) {
> + PyErr_SetString(socket_error, "no interface with this index");
> + return NULL;
> + }
> +
> + return PyBytes_FromString(name);
> +}
> +
> +PyDoc_STRVAR(if_indextoname_doc,
> +"if_indextoname(if_index)\n\
> +\n\
> +Returns the interface name corresponding to the interface index if_index.");

Ditto.
> +
> +#endif /* HAVE_IF_NAMEINDEX */
> +
> +
> /* List of functions exported by this module. */
>> static PyMethodDef socket_methods[] = {
> @@ -4322,6 +4424,14 @@
> METH_NOARGS, getdefaulttimeout_doc},
> {"setdefaulttimeout", socket_setdefaulttimeout,
> METH_O, setdefaulttimeout_doc},
> +#ifdef HAVE_IF_NAMEINDEX
> + {"if_nameindex", socket_if_nameindex,
> + METH_NOARGS, if_nameindex_doc},
> + {"if_nametoindex", socket_if_nametoindex,
> + METH_O, if_nametoindex_doc},
> + {"if_indextoname", socket_if_indextoname,
> + METH_O, if_indextoname_doc},
> +#endif
> {NULL, NULL} /* Sentinel */
> };
>> diff --git a/configure b/configure
> --- a/configure
> +++ b/configure
> [...]

Best Regards,
Ezio Melotti