bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239

pablogsal · 2018-01-19T01:55:29Z

https://bugs.python.org/issue31368

njsmith

Some comments below. Also: should we expose pwritev2 (and maybe pwritev) at the same time? In fact half the constants that this patch adds are only usable with pwritev2.

njsmith · 2018-01-19T04:06:09Z

Modules/posixmodule.c

+    if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1;
+    if (PyModule_AddIntConstant(m, "RWF_SYNC", RWF_SYNC)) return -1;
+    if (PyModule_AddIntConstant(m, "RWF_NOWAIT", RWF_NOWAIT)) return -1;
+#endif


These constants were added at different times, so they should be checked for individually, like:

#ifdef RWF_DSYNC if (PyModule_AddIntConstant(m, "RWF_DSYNC", RWF_DSYNC)) return -1; #endif #ifdef RWF_HIPRI if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1; #endif ...

Also EOPNOTSUPP flag might be important to add?

That's already available as errno.EOPNOTSUPP.

njsmith · 2018-01-19T04:08:31Z

Modules/clinic/posixmodule.c.h

@@ -3705,6 +3705,50 @@ os_pread(PyObject *module, PyObject *const *args, Py_ssize_t nargs)

 #endif /* defined(HAVE_PREAD) */

+#if defined(HAVE_PREADV2)


You use HAVE_PREADV2, but I don't see anything to actually set it. Did you forget to update configure.ac?

njsmith · 2018-01-19T04:16:26Z

Modules/clinic/posixmodule.c.h

+#if defined(HAVE_PREADV2)
+
+PyDoc_STRVAR(os_preadv2__doc__,
+"preadv2($module, fd, buffers, offset, flags, /)\n"


API question: preadv2 only exists because regular preadv was missing a flags argument, and in C the only way to add an argument is to make a new function. In python we don't have that problem. So an alternative would be to make this os.preadv(fd, buffers, offset[, flags]), and have it call either preadv or preadv2 as appropriate. (preadv2(..., flags=0) is equivalent to preadv.)

I kind of like this better, since (a) exposing preadv is probably a good idea, even on systems that don't have preadv2, (b) it reduces clutter in os. OTOH the same thing happened for pipe and pipe2, and it looks like we exposed those as two independent functions in os, so ¯\(ツ)/¯

https://www.gnu.org/software/libc/manual/html_mono/libc.html#index-preadv2 seems to support @njsmith statement.

@njsmith Thanks for the comment! CPython only exposes pread and readv and currently, it does not expose preadv. Is your suggestion to implement preadv2 as preadv and call the former if flags are provided and the later (preadv) in ther case?

I am asking this because initially I thought you were saying that preadv was available and we should reuse the function

Right, I'm doing the classic pull request reviewer move of trying to trick you into doing extra work while you're there :-). We don't currently expose preadv, but it's obviously a useful thing, so I'm suggesting you add a preadv wrapper, and then add preadv2 support to that. If we add preadv2 now alone, then we'll probably end up adding preadv later, and thus have a messier api in the end.

BTW, it's probably simpler to structure the code like (forgive my mixed Python/c pseudocode):

#if defined(HAVE_PREADV) || defined (HAVE_PREADV2) def preadv(fd, offset, buffers, flags=0): #ifdef HAVE_PREADV2 return preadv2(...) #else if flags != 0: raise ... return preadv(...) #endif

Since preadv2 has a superset of the functionality of preadv, when it's available you can use it for all flags values, including 0.

YoSTEALTH · 2018-01-19T04:29:16Z

Wow what a great start by @pablogsal thank you for creating this patch :)

pablogsal · 2018-01-19T21:49:47Z

@njsmith I am working on fixing your comments. I would prefer to implement pwritev2 and pwritev in another PR if you don't mind unless you advise otherwise. :)

njsmith · 2018-01-20T00:05:37Z

I would prefer to implement pwritev2 and pwritev in another PR if you don't mind unless you advise otherwise. :)

I'm not a core dev so I can't give a definitive answer, but it seems fine to me.

njsmith

Otherwise LGTM

njsmith · 2018-01-20T00:54:27Z

Modules/posixmodule.c

+
+    if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0)
+        return -1;
+    #ifdef HAVE_PREADV2


Style nit: I'm not sure why, but the convention in C is that preprocessor directives are never indented, so like:

if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0) return -1; #ifdef HAVE_PREADV2 do {

(and similarly elsewhere).

njsmith · 2018-01-20T00:56:14Z

Modules/posixmodule.c

+    } while (n < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals()));
+    #endif
+
+    iov_cleanup(iov, buf, cnt);


If we exit early (e.g. due to flags != 0), then this cleanup function doesn't get called. Is that OK?

I have corrected it in 4ddbca9. I could not use easily a goto label to have a clean exit section due to the check for async error in the signal handler so I just clean before exiting if flags != 0.

csabella · 2018-01-21T21:48:02Z

Does this need to be added to the os doc page? pread is there.

YoSTEALTH · 2018-01-21T22:30:09Z

In my opinion, yes. It needs to be for os.preadv() with a note mentioned it works for preadv2 as well.

# Note-1 (function)

os.preadv(fd, buffers, offset=0, flags=0)
...

# Note-2 (flags)

os.RWF_DSYNC
os.RWF_HIPRI
os.RWF_SYNC
os.RWF_NOWAIT

You can read more about preadv & preadv2 here: https://www.gnu.org/software/libc/manual/html_node/I_002fO-Primitives.html

pablogsal · 2018-01-22T20:40:07Z

@csabella Thanks for the comment! I was waiting to check that everyone was happy with the interface but I suppose there isn't much freedom given the current implementations of readv and pread. I have added the function to the os module Docs.

pablogsal · 2018-01-22T20:40:34Z

@YoSTEALTH Thanks for the links and comments!

YoSTEALTH · 2018-01-22T21:19:31Z

@pablogsal there are few places where preadv2 is mentioned including title "Expose preadv2 in the os module" shouldn't it be "Expose preadv (preadv2) in the os module" also under NEWS. Simple typo fixes, might lead to confusion.

Also what about "RWF_DSYNC" & "RWF_SYNC" flag reference (docs) are they covered somewhere else?

Thanks

csabella · 2018-01-22T21:23:30Z

Doc/library/os.rst

+
+   Combines the functionality of :func:`os.readv` and :func:`os.pread`. It
+   performs the same task as :func:`os.readv`, but adds a fourth argument,
+   offset, which specifies the file offset at which the input operation is


You put asterisks around flags below and I think the first offset should have the name because this is referring to the argument.

Corrected in c7b7285

csabella · 2018-01-22T21:24:03Z

Doc/library/os.rst

@@ -1195,6 +1195,32 @@ or `the MSDN <https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx>`_ on Windo
   .. versionadded:: 3.3


+.. function:: preadv(fd, buffers, offset, flags = None)


I don't know the answer to this, but should it be flags=None with no spaces?

Actually, I just realized this is implemented in C. I can't really comment on that code, but you have it as flags=0 in the C module. Is None and 0 the same thing?

You are right. The None was because a previous declaration before switching to the default suggested by @njsmith. It should be flags=0. Corrected in c7b7285.

csabella · 2018-01-22T21:27:13Z

Doc/library/os.rst

@@ -1195,6 +1195,32 @@ or `the MSDN <https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx>`_ on Windo
   .. versionadded:: 3.3


+.. function:: preadv(fd, buffers, offset, flags = None)
+
+   Combines the functionality of :func:`os.readv` and :func:`os.pread`. It


I noticed you didn't duplicate the definition of fd and buffers from readv. I think most of docs explain all the parms, even if they are duplicated from another section.

I has hessitant to duplicate because readv is the function described inmediately before this one but I suppose that consistency makes sense. Thanks.

Corrected in c7b7285

vstinner · 2018-01-23T01:17:59Z

Doc/library/os.rst


   The flags argument contains a bitwise OR of zero or more of the following
   flags:

+  Availability: Unix (version 2.6.30), FreeBSD (version 6.0 and newer),


Unix: do you mean Linux?

"FreeBSD (version 6.0 and newer)" may be written "FreeBSD 6 and newer".

Done in 47f65b5

vstinner · 2018-01-23T01:18:45Z

Doc/library/os.rst

+   :func:`~os.pwritev` will call *pwritev2* system call if available, which modifies
+   the behavior on a per-call basis based on the value of the *flags* argument. If
+   the *flags* argument is provided and different than zero, and *pwritev2* is not
+   available, it will raise a :exc:`NotImplementedError`.

   The flags argument contains a bitwise OR of zero or more of the following
   flags:


of which flags? write an explicit list here.

Done in 47f65b5

vstinner · 2018-01-23T01:23:39Z

Modules/posixmodule.c


-    if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0)
+#ifndef HAVE_PREADV2
+    if(flags != 0){


nitpick: add spaces (see PEP 7): "if (flags != 0) {".

Done in 47f65b5

vstinner · 2018-01-23T01:26:12Z

Modules/posixmodule.c

-    if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_SIMPLE) < 0)
+#ifndef HAVE_PWRITEV2
+    if(flags != 0){
+        argument_unavailable_error("pwritev2", "pwritev2() not available in this system");


You misused the API. It should be something like: argument_unavailable_error("pwritev", "flags");

Sorry, my fault. Is 2AM here and I clearly should be sleeping. 😪

vstinner · 2018-01-23T01:28:28Z

Doc/library/os.rst

+   :func:`~os.pwritev` will call *pwritev2* system call if available, which modifies
+   the behavior on a per-call basis based on the value of the *flags* argument. If
+   the *flags* argument is provided and different than zero, and *pwritev2* is not
+   available, it will raise a :exc:`NotImplementedError`.


I would move this paragraph to the end, before Availability, and simplify it to one sentence, something like: "The :c:func:pwritev2 function is required to pass flags."

I would prefer to not mention NotImplementedError, it's not documlented for other functions.

Maybe mention in which Linux version is it was added (4.14?).

pablogsal · 2018-01-26T12:32:58Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-26T12:33:00Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner · 2018-01-26T13:05:55Z

Doc/library/os.rst

+.. function:: pwritev(fd, buffers, offset, flags=0)
+
+   Combines the functionality of :func:`os.writev` and :func:`os.pwrite`. It
+   writes the contents of *buffers* to file descriptor *fd*. *buffers* must be


Hum, offset is not documented. You may just write: "writes the contents of buffers to file descriptor fd at offset offset."

vstinner · 2018-01-26T13:07:24Z

Doc/library/os.rst

+   :func:`~os.pwritev2` function is required to pass flags.
+
+   *pwritev2* and *flags* different than zero are available since Linux version
+   4.7 and newer.


nitpick: just "since Linux 4.7" (no "and newer") or "available on Linux 4.7 and newer".

vstinner · 2018-01-26T13:07:55Z

Doc/library/os.rst

+   *pwritev2* and *flags* different than zero are available since Linux version
+   4.7 and newer.
+
+   The :func:`~os.pwritev2` function is required to pass flags.


nitpick: I would move this sentence before describing on which Linux platforms version it is available.

vstinner · 2018-01-26T13:09:17Z

Doc/library/os.rst

+   data into each buffer until it is full and then move on to the next buffer in
+   the sequence to hold the rest of the data. Its fourth argument, *offset*,
+   specifies the file offset at which the input operation is to be performed.
+   :func:`~os.preadv` return the total number of bytes read (which may be less than


may be => can be

vstinner · 2018-01-26T13:10:37Z

Doc/library/os.rst

+
+   :func:`~os.preadv` will call *preadv2* system call if available, which modifies
+   the behavior on a per-call basis based on the value of the *flags* argument. The
+   :func:`~os.preadv2` function is required to pass flags.


os.preadv2? I guess that you mean the C function preadv2, no? os.preadv2 doesn't exist.

I suggest "The C :c:func:preadv2 function is required ...".

vstinner · 2018-01-26T13:16:57Z

Lib/test/test_posix.py

+        finally:
+            os.close(fd)
+
+    @unittest.skipUnless(hasattr(posix, 'RWF_HIPRI'), "test needs posix.preadv2()")


posix.preadv2 doesn't exist. I suggest "test needs posix.RWF_HIPRI".

I also suggest to rename the test to test_preadv_flags() to be closer to the Python API than the C internals.

vstinner · 2018-01-26T13:19:49Z

Lib/test/test_posix.py

+    def test_pwritev(self):
+        fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
+        try:
+            n = os.pwritev(fd, [b'test1tt2t3'],2)


I would prefer to test writing multiple buffers than a single buffer. Maybe write [b'test1', b'tt2', b't3']?

I would prefer to write explicitly the first 2 bytes, but then seek back at offset 0 before calling os.pwritev(). I don't want to test in Python the behaviour of pwritev() for non-offset offset with an empty file. Python should test its binding, not the exact behaviour of the OS.

For example:
write(b'XX')
seek(0)
pwrite(..., offset=2)
assert read() == b'XX...'

vstinner · 2018-01-26T13:20:52Z

Lib/test/test_posix.py

+
+
+    @unittest.skipUnless(hasattr(posix, 'os.RWF_SYNC'), "test needs posix.pwritev2()")
+    def test_pwritev2(self):


ditto, rename to test_pwritev_flags() and change the skip message to not mention a function that doesn't exist.

vstinner · 2018-01-26T13:22:13Z

Modules/posixmodule.c

+
+    iov_cleanup(iov, buf, cnt);
+    if (n < 0) {
+        if (!async_err)


PEP 7 coding style: add { ... }

vstinner · 2018-01-26T13:22:26Z

Modules/posixmodule.c

+
+    iov_cleanup(iov, buf, cnt);
+    if (result < 0) {
+        if (!async_err)


ditto: add { ... }

bedevere-bot · 2018-01-26T13:22:43Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2018-01-26T18:56:15Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-26T18:56:17Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

Another review.

Most comments are on the documentation and coding style, so IMHO we are now very close to a ready PR ;-)

vstinner · 2018-01-27T00:06:04Z

Doc/library/os.rst

+   (sysconf() value SC_IOV_MAX) on the number of buffers that can be used.
+   :func:`~os.pwritev` writes the contents of each object to the file descriptor
+   and returns the total number of bytes written.
+


a single empty line is enough.

vstinner · 2018-01-27T00:07:10Z

Doc/library/os.rst

+
+   :func:`~os.pwritev` will call *pwritev2* system call if available, which modifies
+   the behavior on a per-call basis based on the value of the *flags* argument. The
+   :func:`~os.pwritev2` function is required to pass flags.


No, there is no :func:~os.pwritev2 function, stop creating links to it.

I suggested "The C function :func:pwritev2", or you can just write pwrite2().

vstinner · 2018-01-27T00:08:29Z

Doc/library/os.rst

+   - RWF_SYNC
+
+   :func:`~os.pwritev` will call *pwritev2* system call if available, which modifies
+   the behavior on a per-call basis based on the value of the *flags* argument. The


I'm not sure that ", which modifies the behavior on a per-call basis based on the value of the flags argument." is needed. Nobody expects a flag passed to a function to have an effect on following calls. And the usage of pwrite2() doesn't mean that the caller must pass a non-zero flags. This sentence is confusing.

vstinner · 2018-01-27T00:09:25Z

Doc/library/os.rst

+   the behavior on a per-call basis based on the value of the *flags* argument. The
+   :func:`~os.pwritev2` function is required to pass flags.
+
+   The :func:`~os.pwritev2` function is required to pass flags.


ditto, don't write ":func:~os.pwritev2 function" but "C :c:func:pwritev2 function" or just pwrite2().

Note the "c" prefix before "func".

vstinner · 2018-01-27T00:11:57Z

Doc/library/os.rst

+   The :func:`~os.pwritev2` function is required to pass flags.
+
+   *pwritev2* and *flags* different than zero are available since Linux version
+   4.7.


"The :func:~os.pwritev2 function is required to pass flags. pwritev2 and flags different than zero are available since Linux version 4.7."

I'm not sure that the Python has to mention pwritev2(). It's confusing and useless. I don't think that the documentation of other os function lists each internal called C functions.

I suggest something simpler like: "Using non-zero flags requires Linux 4.7 or newer."

vstinner · 2018-01-27T00:18:52Z

Lib/test/test_posix.py

+        finally:
+            os.close(fd)
+
+


ditto: remove this empty line

vstinner · 2018-01-27T00:19:08Z

Lib/test/test_posix.py

+    def test_pwritev(self):
+        fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT)
+        try:
+            os.write(fd,b"xx")


PEP 8: add a space after ","

vstinner · 2018-01-27T00:20:26Z

Misc/NEWS.d/next/Core and Builtins/2018-01-19-01-54-22.bpo-31368.kzKqUR.rst

@@ -0,0 +1,2 @@
+Expose preadv (preadv2) and pwritev (pwritev2) system calls in the os module.


I suggest: Add os.preadv() and os.pwritev() functions.

Again, I don't think that it's needed to mention the underlying C functions.

vstinner · 2018-01-27T00:21:03Z

Modules/posixmodule.c

+Read a number of bytes from a file descriptor starting at a particular offset.
+
+Read length bytes from file descriptor fd, starting at offset bytes from
+the beginning of the file.  The file offset remains unchanged.


Wait... is this description correct? it seems to be copied from two other functions?

vstinner · 2018-01-27T00:21:39Z

Modules/posixmodule.c

+    flags: int = 0
+    /
+
+Write bytes to a file descriptor starting at a particular offset.


Ditto: this docstring seems wrong.

pablogsal · 2018-01-27T13:22:38Z

I have addressed all the feedback. The only thing I am not very sure is the description of the flags as that description is now the only place that explicitly mentions preadv2 and pwritev2. I am not sure what is the best course of action here: if somehow mention only preadv and pwritev or to leave as it is (as they are mentioning the system call so maybe this is less confusing).

I have removed all other mentions of the C calls, please check that those paragraphs are clear enough and nothing is missed.

pablogsal · 2018-01-27T13:24:12Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-27T13:24:14Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

bedevere-bot · 2018-01-27T16:16:39Z

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

vstinner · 2018-01-27T16:19:22Z

I'm not 100% satisfied by the documentation but I prefer to merge this PR right now to get it before 3.7beta1, feature freeze of Python 3.7.

I may propose a different PR to enhance the doc.

Thanks Pablo for this addition!

YoSTEALTH · 2018-01-27T19:23:26Z

Thank you Pablo Galindo for your amazingly hard work. Also Victor Stinner for your guidance, thanks you :)

the-knights-who-say-ni added the CLA signed label Jan 19, 2018

bedevere-bot added the awaiting review label Jan 19, 2018

pablogsal force-pushed the bpo31368 branch from 540b0fb to 67dcabf Compare January 19, 2018 02:07

njsmith reviewed Jan 19, 2018

View reviewed changes

pablogsal force-pushed the bpo31368 branch 6 times, most recently from a8ec27f to 52c0bfb Compare January 19, 2018 22:47

njsmith reviewed Jan 20, 2018

View reviewed changes

pablogsal added 6 commits January 20, 2018 16:33

Expose preadv2 in os module

ea21128

Add News entry

4570976

Add correct guards for the presence of preadv2

21a3004

Implement check for individual constants

8a2a2a3

Implemented preadv and join implementation to preadv2

c0c53dc

Clean iov on error

4ddbca9

pablogsal force-pushed the bpo31368 branch from cbcea73 to 4ddbca9 Compare January 20, 2018 16:35

Add documentation in the os module

cac62ac

csabella reviewed Jan 22, 2018

View reviewed changes

bedevere-bot added the awaiting change review label Jan 23, 2018

vstinner reviewed Jan 23, 2018

View reviewed changes

Correct style, update docs, fix misused API

47f65b5

vstinner requested changes Jan 26, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 26, 2018

pablogsal added 2 commits January 26, 2018 11:57

Improve tests, clean names and correct style

87c6970

Correct documentation

cb74d38

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 26, 2018

vstinner reviewed Jan 27, 2018

View reviewed changes

pablogsal force-pushed the bpo31368 branch from a8cf3e3 to be7f95b Compare January 27, 2018 13:02

Correct typos, docstring and improve Docs

5563b8d

pablogsal force-pushed the bpo31368 branch from be7f95b to 5563b8d Compare January 27, 2018 13:16

vstinner merged commit 4defba3 into python:master Jan 27, 2018

bedevere-bot removed the awaiting change review label Jan 27, 2018

pablogsal deleted the bpo31368 branch January 27, 2018 19:24

pablogsal restored the bpo31368 branch January 29, 2018 21:03

pablogsal deleted the bpo31368 branch January 29, 2018 21:03

		@@ -3705,6 +3705,50 @@ os_pread(PyObject module, PyObject const *args, Py_ssize_t nargs)

		#endif /* defined(HAVE_PREAD) */

		#if defined(HAVE_PREADV2)

		@@ -1195,6 +1195,32 @@ or `the MSDN <https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx>`_ on Windo
		.. versionadded:: 3.3


		.. function:: preadv(fd, buffers, offset, flags = None)



		@unittest.skipUnless(hasattr(posix, 'os.RWF_SYNC'), "test needs posix.pwritev2()")
		def test_pwritev2(self):

		@@ -0,0 +1,2 @@
		Expose preadv (preadv2) and pwritev (pwritev2) system calls in the os module.

bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239

bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239

Conversation

pablogsal commented Jan 19, 2018 • edited by bedevere-bot Loading

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoSTEALTH commented Jan 19, 2018

pablogsal commented Jan 19, 2018

njsmith commented Jan 20, 2018

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Jan 21, 2018

YoSTEALTH commented Jan 21, 2018 • edited Loading

# Note-1 (function)

# Note-2 (flags)

pablogsal commented Jan 22, 2018

pablogsal commented Jan 22, 2018

YoSTEALTH commented Jan 22, 2018

Choose a reason for hiding this comment

pablogsal Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

csabella Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

pablogsal Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 26, 2018

bedevere-bot commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 26, 2018

pablogsal commented Jan 26, 2018

bedevere-bot commented Jan 26, 2018

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 27, 2018 • edited Loading

pablogsal commented Jan 27, 2018

bedevere-bot commented Jan 27, 2018

pablogsal commented Jan 19, 2018 •

edited by bedevere-bot

Loading

pablogsal Jan 19, 2018 •

edited

Loading

YoSTEALTH commented Jan 21, 2018 •

edited

Loading

pablogsal Jan 22, 2018 •

edited

Loading

csabella Jan 22, 2018 •

edited

Loading

pablogsal Jan 22, 2018 •

edited

Loading

pablogsal Jan 22, 2018 •

edited

Loading

pablogsal commented Jan 27, 2018 •

edited

Loading