[Python-checkins] r46300 - in python/trunk: Lib/socket.py Lib/test/test_socket.py Lib/test/test_struct.py Modules/_struct.c Modules/arraymodule.c Modules/socketmodule.c

[Neal Norwitz]

In the future, please try to do whitespace changes in a separate
checkin.  Also, unrelated changes should go in a separate checkin.
Don't forget AMK's comment about adding doc.  Other comments inline.

On 5/26/06, martin.blais <python-checkins at python.org> wrote:
> Author: martin.blais
> Date: Fri May 26 14:03:27 2006
> New Revision: 46300
>
> Modified:
>    python/trunk/Lib/socket.py
>    python/trunk/Lib/test/test_socket.py
>    python/trunk/Lib/test/test_struct.py
>    python/trunk/Modules/_struct.c
>    python/trunk/Modules/arraymodule.c
>    python/trunk/Modules/socketmodule.c
> Log:
> Support for buffer protocol for socket and struct.
>
> * Added socket.recv_buf() and socket.recvfrom_buf() methods, that use the buffer
>   protocol (send and sendto already did).
>
> * Added struct.pack_to(), that is the corresponding buffer compatible method to
>   unpack_from().
>
> * Fixed minor typos in arraymodule.
>
>
>
>
> Modified: python/trunk/Lib/test/test_struct.py
> ==============================================================================
> --- python/trunk/Lib/test/test_struct.py        (original)
> +++ python/trunk/Lib/test/test_struct.py        Fri May 26 14:03:27 2006
>  def test_1229380():
>      for endian in ('', '>', '<'):
>          for cls in (int, long):
> @@ -478,3 +456,60 @@
>  if 0:
>      # TODO: bug #1229380
>      test_1229380()
> +
> +class PackBufferTestCase(unittest.TestCase):

[...]

> +def test_main():
> +    test.test_support.run_unittest(PackBufferTestCase)
> +
> +if __name__ == "__main__":
> +    test_main()

I don't think this really works to have both tests that follow the
old-style (test on import) and unittest.  It will create problems for
finding refleaks and possibly other parts of regtest.  It would be
great if you could convert the rest of the tests in this file to use
unittest.  Barring that, I think the new test should use the old
style.  Or you could create a new file.


> Modified: python/trunk/Modules/_struct.c
> ==============================================================================
> --- python/trunk/Modules/_struct.c      (original)
> +++ python/trunk/Modules/_struct.c      Fri May 26 14:03:27 2006
> +PyDoc_STRVAR(s_pack__doc__,
> +"pack(v1, v2, ...) -> string\n\
> +\n\
> +Return a string containing values v1, v2, ... packed according to this\n\
> +Struct's format. See struct.__doc__ for more on format strings.");
> +
> +static PyObject *
> +s_pack(PyObject *self, PyObject *args)
> +{
> +       PyStructObject *soself;
> +       PyObject *result;
> +
> +       /* Validate arguments. */
> +       soself = (PyStructObject *)self;
> +       assert(PyStruct_Check(self));
> +       assert(soself->s_codes != NULL);
> +       if (args == NULL || !PyTuple_Check(args) ||
> +           PyTuple_GET_SIZE(args) != soself->s_len)

I realize this code was here before in a different place, but args is
guaranteed to be a non-NULL tuple, so there's no reason to check
those.  You still need to verify the size.

> +static PyObject *
> +s_pack_to(PyObject *self, PyObject *args)
> +{
> +       PyStructObject *soself;
> +       char *buffer;
> +       Py_ssize_t buffer_len, offset;
> +
> +       /* Validate arguments.  +1 is for the first arg as buffer. */
> +       soself = (PyStructObject *)self;
> +       assert(PyStruct_Check(self));
> +       assert(soself->s_codes != NULL);
> +       if (args == NULL || !PyTuple_Check(args) ||
> +           PyTuple_GET_SIZE(args) != (soself->s_len + 2))

Same deal.

> +       {
> +               PyErr_Format(StructError,
> +                            "pack_to requires exactly %d arguments",
> +                            (soself->s_len + 2));

s_len is a Py_ssize_t, the format needs to be %zd.  This is probably
elsewhere too.

> +               return NULL;
> +       }
> +
> +       /* Extract a writable memory buffer from the first argument */
> +        if ( PyObject_AsWriteBuffer(PyTuple_GET_ITEM(args, 0),
> +                                    (void**)&buffer, &buffer_len) == -1 ) {
> +               return NULL;
> +        }
> +        assert( buffer_len >= 0 );
> +
> +       /* Extract the offset from the first argument */
> +       offset = PyInt_AsLong(PyTuple_GET_ITEM(args, 1));

AsLong or AsSizeT?

> +
> +       /* Support negative offsets. */
> +       if (offset < 0)
> +               offset += buffer_len;
> +
> +       /* Check boundaries */
> +       if (offset < 0 || (buffer_len - offset) < soself->s_size) {
> +               PyErr_Format(StructError,
> +                            "pack_to requires a buffer of at least %d bytes",
> +                            soself->s_size);

%zd

> @@ -1400,6 +1479,7 @@
>
>  static struct PyMethodDef s_methods[] = {
>         {"pack",        (PyCFunction)s_pack,            METH_VARARGS, s_pack__doc__},
> +       {"pack_to",     (PyCFunction)s_pack_to,         METH_VARARGS, s_pack_to__doc__},

s_pack_to doesn't need to be casted to a PyCFunction, it's signature
is already correct.  You can probably remove all the casts.

>         {"unpack",      (PyCFunction)s_unpack,          METH_O, s_unpack__doc__},
>         {"unpack_from", (PyCFunction)s_unpack_from,     METH_KEYWORDS, s_unpack_from__doc__},
>         {NULL,   NULL}          /* sentinel */


> Modified: python/trunk/Modules/socketmodule.c
> ==============================================================================
> --- python/trunk/Modules/socketmodule.c (original)
> +++ python/trunk/Modules/socketmodule.c Fri May 26 14:03:27 2006

> +static PyObject *
> +sock_recv(PySocketSockObject *s, PyObject *args)
> +{
> +       int recvlen, flags = 0, outlen;
> +       PyObject *buf;
> +
> +       if (!PyArg_ParseTuple(args, "i|i:recv", &recvlen, &flags))
> +               return NULL;

Note:  it looks like recv takes size_t for the len and returns an
ssize_t on Unix.  It might be nice to use the same interface here.
This would apply to the other network APIs too.

> +       if (recvlen < 0) {
> +               PyErr_SetString(PyExc_ValueError,
> +                               "negative buffersize in recv");
> +               return NULL;
> +       }
> +
> +       /* Allocate a new string. */
> +       buf = PyString_FromStringAndSize((char *) 0, recvlen);
> +       if (buf == NULL)
> +               return NULL;
> +
> +       /* Call the guts */
> +       outlen = sock_recv_guts(s, PyString_AsString(buf), recvlen, flags);

Should use AS_STRING since this will return a valid pointer.  It's
faster and better static analysis can be done on it.  It also points
out that you know this is a string.

> +static PyObject *
> +sock_recvfrom_buf(PySocketSockObject *s, PyObject *args, PyObject* kwds)

[...]

> +       readlen = sock_recvfrom_guts(s, buf, recvlen, flags, &addr);
> +       if (readlen < 0) {
> +               /* Return an error */
> +               goto finally;
> +       }
> +
> +       /* Return the number of bytes read and the address.  Note that we do
> +          not do anything special here in the case that readlen < recvlen. */
> +       ret = PyTuple_Pack(2, PyInt_FromLong(readlen), addr);
> +
> +finally:
> +       Py_XDECREF(addr);
> +       return ret;

Why not just do the Tuple_Pack if readlne >= 0?  Hmmm, doesn't this leak a ref?

n