net: BSD Sockets like API initial implementation #498

pfalcon · 2017-06-14T22:52:20Z

This implements BSD Sockets like API, blocking mode, functions: socket(), bind(), connect(), listen(), accept(), recv(), send(), per https://jira.zephyrproject.org/browse/ZEP-2226 .

A sample echo server is supplied (portable to (well, from) POSIX systems) and a simple test for UDP functionality.

pfalcon · 2017-06-14T22:57:58Z

@jukkar , @tbursztyka , @lpereira , @Likailiu : Please review.

There're many commits, something definitely needs to be squashed, I'm open to suggestions how much. Other thing requiring decision is "WIP: kernel: fifo: Add peek_head/peek_tail accessors." - whether to turn these functions into true kernel fifo apis, or make private socket functions.

pfalcon · 2017-06-15T17:23:02Z

@askawu: Please review too.

pfalcon · 2017-06-15T17:26:47Z

Shippable fails (so far) due:

Commit 279ddf5839:
1: T5 Title contains the word 'wip' (case-insensitive)

That's "WIP: kernel: fifo: Add peek_head/peek_tail accessors." commit, about which I ask suggestions above.

pfalcon · 2017-06-15T19:58:59Z

Looking that we have tests/net/lib/, maybe should create tests/net/sockets/ to group socket tests?

jukkar

Minor things needs tweaking, but looks very good as a whole.

jukkar · 2017-06-16T13:10:59Z

samples/net/socket_echo/prj_qemu_x86.conf

+CONFIG_NET_SLIP_TAP=y
+CONFIG_TEST_RANDOM_GENERATOR=y
+
+# Without CONFIG_NET_BUF_LOG printf() doesn't work


I do not really understand how net_buf logging could affect printf(), could you elaborate the issue more.

BTW, typically I have this setting in my prj.conf files

CONFIG_NET_LOG=y
CONFIG_SYS_LOG_NET_LEVEL=2

and then setting relevant net debug options to "y" and increasing CONFIG_SYS_LOG_NET_LEVEL to 4, will then start to print debug output.

I do not really understand how net_buf logging could affect printf(), could you elaborate the issue more.

I don't understand either, and yet it's like that - w/o CONFIG_NET_BUF_LOG=y, printf() output is not shown (network echo works). I did 10-15 mins of "genetic algorithm" debugging, semi-randomly trying other logging and output options and only CONFIG_NET_BUF_LOG looked like a culprit. Didn't debug further, as you may imagine, that can be quite a timesink ;-)

jukkar · 2017-06-16T13:16:45Z

samples/net/socket_echo/prj_qemu_x86.conf

+CONFIG_TEST_RANDOM_GENERATOR=y
+
+# Without CONFIG_NET_BUF_LOG printf() doesn't work
+CONFIG_NET_BUF_LOG=y


Normally one does not need to enable this unless one is debugging the actual net_buf implementation.

jukkar · 2017-06-16T13:20:12Z

samples/net/socket_echo/src/socket_echo.c

+
+	serv = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+
+	bind_addr.sin_family = AF_INET;


It would be nice if this app would support also IPv6. This can be enabled later, if no time to do that now.

So (also partially answering @lpereira's comment below), the primary purpose of this sample was to show that the same code (modulo includes of course) can be used both on POSIX and Zephyr. Turns out, I missed to include "Makefile.posix" to make that obvious, I'll add it. Then, Linux for example supports "dual stack" sockets which can be accessed via IPv4 or IPv6, but I don't think we have that feature in Zephyr. Otherwise, to support IPv4 and IPv6 at the same time, there would be needed 2 sockets, and poll()/select(), which are scheduled so "stage 3" of Sockets implementation (this patch being stage 1).

Perhaps you mean either IPv4 or IPv6? That can be done, though again, the idea was to show how sockets can be portable between POSIX and Zephyr, without extra #ifdef noise.

I meant having dual IPv4 and IPv6 at the same time. But it requires two sockets at the moment and support for poll/select as you said, so we can postpone this for later stage.

jukkar · 2017-06-16T13:22:52Z

subsys/net/lib/sockets/sockets.c

+		user_data);
+
+	/* if pkt == NULL, EOF */
+	if (pkt == NULL) {


It would be better to have "if (!pkt) {" here, so that the code has unified look with rest of the IP stack.

jukkar · 2017-06-16T13:22:58Z

subsys/net/lib/sockets/sockets.c

+	if (pkt == NULL) {
+		struct net_pkt *last_pkt = _k_fifo_peek_tail(&ctx->recv_q);
+
+		if (last_pkt == NULL) {


Fixed (other cases too).

jukkar · 2017-06-16T13:24:14Z

subsys/net/lib/sockets/sockets.c

+			 */
+			sock_set_eof(ctx);
+			k_fifo_cancel_wait(&ctx->recv_q);
+			NET_DBG("Marked socket %p as peer-closed\n", ctx);


No need to put \n at the end of the strings in NET_DBG(), as those strings will have \n automatically added. Please check also other calls to NET_DBG() in your patches for duplicate \n

Fixed, was printk() initially...

jukkar · 2017-06-16T13:28:09Z

subsys/net/lib/sockets/sockets.c

+
+ssize_t zsock_recv(int sock, void *buf, size_t max_len, int flags)
+{
+	(void)flags;


You can use ARG_UNUSED(flags); here

jukkar · 2017-06-16T13:29:19Z

tests/net/socket_udp/prj.conf

+CONFIG_NET_SLIP_TAP=y
+CONFIG_TEST_RANDOM_GENERATOR=y
+
+


extra empty line

lpereira · 2017-06-16T20:24:55Z

subsys/net/lib/sockets/sockets.c

+	if (sock_type == SOCK_DGRAM) {
+		__ASSERT(0, "DGRAM is not yet handled");
+	} else if (sock_type == SOCK_STREAM) {
+		do {


You can move this do { ... } while() to a function and return zsock_recv_stream(ctx, buf, max_len, flags);, reducing one indentation level.

I can. So, should I? (I don't expect source-level issues, like need to re-do some work in this function, but I'm not sure if a compiler will readily inline such a function, which in turn may lead to higher stack usage, etc.)

GCC does tail-call optimization, so there's no need to worry about increased stack usage. Essentially, if a function does return function(...), instead of generating a CALL instruction (or their equivalent), it'll generate a JMP instruction (or their equivalent) after setting up the parameters. It's very likely that there won't be any special code for the parameters, as it might be able to reuse the ones in the current call frame.

GCC does tail-call optimization

GCC does tail-call optimization depending on the arch. For example, last time I looked re: that at gcc-xtensa, it didn't do it. Anyway, I assume it makes sense to try that refactor.

lpereira · 2017-06-16T20:32:18Z

From a cursory glance, the patchset looks fine. I'll carve some time to review it more carefully.

For some future work, it would be nice to make something similar to poll() or select(). Select is going to be tricky, considering that the fd is just a pointer in disguise, but poll() might be doable.

As far as samples-for-the-future goes, having a "forking" server would be interesting as well as having something like a thread pool with a shared FIFO to service clients.

pfalcon · 2017-06-20T14:48:52Z

@lpereira :

For some future work, it would be nice to make something similar to poll() or select(). Select is going to be tricky, considering that the fd is just a pointer in disguise, but poll() might be doable.

poll/select is planned, for "stage 3" of the implementation, GH-3664, stage 2 non-blocking support (GH-3663), because realistically, socket poll functionality becomes useful only with non-blocking mode. Indeed, select() will be tricky, so in my plan, I offload it for future "generic POSIX API" work, which also would include read()/write() support for socket objects, etc. poll() should be both good enough and more efficient, though even it would add enough bloat to this implementation.

As far as samples-for-the-future goes, having a "forking" server would be interesting as well as having something like a thread pool with a shared FIFO to service clients.

I assume mentioning of "forking" server is a joke - realistically, we'll never have fork() or vfork() in Zephyr ;-). As I mentioned above, the idea of the current socket_echo sample is to show that it's possible to write apps portable between POSIX and Zephyr, so I didn't want to use Zephyr-specific threading. That again can be addressed by more generic POSIX API advent which would include Pthreads support. Anyway, more examples (or tests) of different kinds can be added later.

Thanks for reviewing!

pfalcon · 2017-06-20T14:58:31Z

Some updates:

samples/net/socket_echo: Added Makefile.posix which allows to build the sample (very same source code) on Linux, etc.
tests/net/socket_udp: Converted to tests/net/socket/udp, in anticipation that there will be more socket tests.

pfalcon · 2017-06-20T15:06:05Z

I don't understand either, and yet it's like that - w/o CONFIG_NET_BUF_LOG=y, printf() output is not shown (network echo works).

So, previously this was tested on qemu_x86, now confirmed that on qemu_cortex_m3 it leads to the same behavior.

pfalcon · 2017-06-20T15:56:35Z

GCC does tail-call optimization depending on the arch. For example, last time I looked re: that at gcc-xtensa, it didn't do it. Anyway, I assume it makes sense to try that refactor.

Ok, some stats:

code size of sockets.o:
qemu_x86: before: 1075, after: 1072, new zsock_recb_stream() is inlined
qemu_cortex_m3: before: 814, after: 826, new zsock_recb_stream() is inlined
qemu_xtensa: before: 781, after: 780, new zsock_recb_stream() is inlined
em_starterkit: before: 928, after: 928, new zsock_recb_stream() is inlined

All in all, single use of static inline function is properly inlined in all cases (which can be "d'oh", but knowing gcc, seeing's believing), and in few cases even leads to surprising reduction in code size, though still noticeable increase for something, which is still relatively minor though. So, I'm rebasing patchset to use separate zsock_recv_stream() function.

GAnthony · 2017-06-21T00:02:12Z

include/net/socket.h

+int zsock_listen(int sock, int backlog);
+int zsock_accept(int sock, struct sockaddr *addr, socklen_t *addrlen);
+ssize_t zsock_send(int sock, const void *buf, size_t len, int flags);
+ssize_t zsock_recv(int sock, void *buf, size_t max_len, int flags);


Would it be logical to place these definitions in a zsocket.h header, to be included by Zephyr applications wanting to use the native Zephyr socket symbols, and leave socket.h to define the POSIX only symbols, as below?

I don't know, this turns one header into two, and reading literally, puts a networking-related header outside net/. Why would this be better? As it stands now, everything socket-related is in one header, but apps can configure subset of the functionality needed, like with the rest of subsystems in Zephyr.

A few reasons:

It's often a standard module convention to have symbols declared in their own header. Makes it easy to find things. (I know, Zephyr doesn't always follow this convention).

These are Zephyr APIs, which may have divergence from true POSIX APIs (declared in socket.h).

It makes it easier to implement a new socket provider for the socket APIs for offloading. zsocket could just be one (the default) socket provider. Socket.h (or socket.c) would be the switch layer.

I assumed zsocket.h would also go into net/.

It's often a standard module convention to have symbols declared in their own header. Makes it easy to find things.

Well, that's actually mu biggest concern - if I'd see socket.h, I would never think there's also zsocket.h, or vice-versa. Zephyr doesn't split IPv4 and IPv6 headers. lwIP defines both native prefixed functions (lwip_recv(), etc.) and POSIXy macros in the same header, sockets.h. And in this case, it's also the single module, it just defines 2 API sets, one being alias of another (but it's the same API).

These are Zephyr APIs, which may have divergence from true POSIX APIs (declared in socket.h).

Well, there're no "true POSIX APIs", only "BSD Sockets like API" ("like" is important). True POSIX APIs live in <sys/socket.h>, Zephyr's BSD Sockets like API lives in <net/socket.h>.

It makes it easier to implement a new socket provider for the socket APIs for offloading. zsocket could just be one (the default) socket provider. Socket.h (or socket.c) would be the switch layer.

Ok, this finally explains why you suggest this change! But why do you want to do it this way? I imagined it would be done just the same way as offloading already implemented in net_context - that each individual zsock_*() will be the place of switching, and that offloading will be supported per netif.

You instead seem to propose that socket offloading will be global, not per netif. Rereading https://jira.zephyrproject.org/browse/ZEP-2271?focusedCommentId=19002&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-19002 , that seems to be what's written in the user story, but if you don't mind, I'd like to get an ack form @jukkar and @tbursztyka that it's ok to proceed with such design right away (vs further discuss it), as it leaves many questions open, e.g. why we have 2 inconsistently implemented offloading mechanisms. Thanks.

Independent of where socket offloading occurs, it just seems it would be logical and future-proof to have zsocket.h defining native zsock functions, and socket.h declaring the socket functions, mapping to zsock. Anyways, just an opinion.

Independent of where socket offloading occurs, it just seems it would be logical and future-proof to have zsocket.h defining native zsock functions, and socket.h declaring the socket functions

Gil, I appreciate the idea, but after additional checking, I don't see such pattern of splitting the same API across 2+ headers to be used in Zephyr. And as I mentioned above, these are indeed intended to be 2 facets of the same API, not 2 different APIs. I'll be happy to make such a change however is there will be 2nd vote for it. Otherwise, well, if it proves to be useful/need, we may need to do such a refactor in the future (e.g. when we'll have better defined POSIX API layer). Thanks for understanding.

Understood.

GAnthony · 2017-06-21T00:02:15Z

include/net/socket.h

+ssize_t zsock_send(int sock, const void *buf, size_t len, int flags);
+ssize_t zsock_recv(int sock, void *buf, size_t max_len, int flags);
+
+#if defined(CONFIG_NET_SOCKETS_POSIX_NAMES)


then could do here:
#include <zsocket.h>

GAnthony · 2017-06-21T00:06:14Z

subsys/net/lib/sockets/sockets.c

+
+#include <net/net_context.h>
+#include <net/net_pkt.h>
+#include <net/socket.h>


This would also be
#include <net/zsocket.h>
as it's calling native Zephyr socket APIs, rather than POSIX socket, close, recv, etc.

GAnthony · 2017-06-21T00:12:27Z

subsys/net/lib/Kbuild

@@ -1,3 +1,4 @@
+obj-$(CONFIG_NET_SOCKETS) += sockets/


Then, maybe the subdirectory should be zsocket/ or zsock/ instead of sockets/?

I'm not sure, why? There're dns, http on the same level, not zdns, zhttp. (There's zoap, yeah, but it's an exception.)

pfalcon · 2017-06-22T12:18:02Z

@andyross , @andrewboie : Can you please check commit titled "WIP: kernel: fifo: Add peek_head/peek_tail accessor" (cc155f7 currently, though may be rebased later), and tell whether you'd prefer these functions, k_fifo_peek_head() and k_fifo_peek_tail() to become part of the general FIFO API (by moving them to <kernel.h>), or if it's better for them to be used in adhoc way just for sockets code.

In this regard it's similar to #26, but that was truly new functionality, so I submitted it ahead, to be sure I can rely on it at all. The functions above are just macro wrappers though, so if there're concerns with making them part of the API, everyone in need can just reinvent them, like done here.

Thanks.

pfalcon · 2017-06-22T14:44:35Z

@dbkinder : Currently this fails with:

                    /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/doc/reference/kconfig/CONFIG_NET_SOCKETS_POSIX_NAMES.rst:8: ERROR: Unknown target name: "zsock".

Context around CONFIG_NET_SOCKETS_POSIX_NAMES.rst:8 is:

.. option:: CONFIG_NET_SOCKETS_POSIX_NAMES:
.. _CONFIG_NET_SOCKETS_POSIX_NAMES:

By default, Sockets API function are prefixed with "zsock_" to avoid
namespacing issues. If this option is enabled, they will be provided
with standard POSIX names like socket(), recv(), and close(), to help

Any idea why it suddenly interpreters plain text as link mark up ans what to do to make it happy? (I already tried single quotes around zsock_, didn't help.)

pfalcon · 2017-06-22T14:53:00Z

@dbkinder : Using backquotes seems to resolve the issue, but that's italics IIRC. Perhaps using double-backquotes (monospace IIRC) probably would make even more sense, it's just unclear why all this markup does in Kconfig files... Please suggest.

This adds Kconfig and build infrastructure and implements zsock_socket() and zsock_close() functions. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

Two changes are required so far: * There's unavoidable need to have a per-socket queue of packets (for data sockets) or pending connections (for listening sockets). These queues share the same space (as a C union). * There's a need to track "EOF" status of connection, synchronized with a queue of pending packets (i.e. EOF status should be processed only when all pending packets are processed). A natural place to store it per-packet then, and we had a "sent" bit which was used only for outgoing packets, recast it as "eof" for incoming socket packets. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

With CONFIG_NET_SOCKETS_POSIX_NAMES=y, "raw" POSIX names like socket(), recv(), close() will be exposed (using macro defines). The close() is the biggest culprit here, because in POSIX it applies to any file descriptor, but in this implementation - only to sockets. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

By moving user_data member at the beginning of structure. With refcount at the beginning, reliable passsing of contexts via FIFO was just impossible. (Queuing contexts to a FIFO is required for BSD Sockets API). Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

As explained in the docstrings, a usecase behind these operations is when other container objects are put in a fifo. The typical processing iteration make take just some data from a container at the head of fifo, with the container still being kept at the fifo, unless it becomes empty, and only then it's removed. Similarly with adding more data - first step may be to try to add more data to a container at the tail of fifo, and only if it's full, add another container to a fifo. The specific usecase these operations are added for is network subsystem processing, where net_buf's and net_pkt's are added to fifo. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

If a socket is closed without reading all data from peer or accepting all pending connection, they will be leaked. So, flush queues explicitly. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

The example source code is POSIX-compatible (modulo include files), i.e. can be built and behaves the same way for Zephyr and a POSIX system (e.g. Linux). Makefile.posix is available for the latter. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon · 2017-06-26T16:18:30Z

@andyross , @andrewboie : Can you please check commit titled "WIP: kernel: fifo: Add peek_head/peek_tail accessor" (cc155f7 currently, though may be rebased later)...

Based on @Vudentz' suggestion at cc155f7#commitcomment-22763881 , this was refactored into kernel.h, please review: dbf743a

pfalcon · 2017-06-29T10:42:23Z

Thanks!

…ject-rtos#498) Also, add a spot for a cleanup function to be registered with modules that will be called by a new zjs_modules_cleanup function. Currently, this is called only in ashell. Also, fix a bug where module instances weren't being released. Signed-off-by: Geoff Gustafson <geoff@linux.intel.com>

…t_double_serializer in 485f2a0 by adding a json_object_put call.

(zephyrproject-rtos#498)

pfalcon mentioned this pull request Jun 14, 2017

[WIP, 1.9] Bootstrapping BSD Sockets like API #153

Closed

jukkar requested changes Jun 16, 2017

View reviewed changes

lpereira reviewed Jun 16, 2017

View reviewed changes

pfalcon force-pushed the net-sockets1 branch from ba510b5 to a9b76f6 Compare June 16, 2017 21:51

pfalcon added the net label Jun 18, 2017

pfalcon force-pushed the net-sockets1 branch from a9b76f6 to e94d076 Compare June 20, 2017 14:56

pfalcon force-pushed the net-sockets1 branch from e94d076 to 157a60c Compare June 20, 2017 15:58

GAnthony mentioned this pull request Jun 20, 2017

Net app API #540

Merged

GAnthony reviewed Jun 21, 2017

View reviewed changes

pfalcon force-pushed the net-sockets1 branch from 157a60c to ab60ec4 Compare June 22, 2017 12:05

pfalcon force-pushed the net-sockets1 branch 2 times, most recently from 4053653 to f1b0460 Compare June 23, 2017 18:21

pfalcon added 3 commits June 26, 2017 17:07

net: sockets: Bootstrap Sockets API implementation

4d35a4d

This adds Kconfig and build infrastructure and implements zsock_socket() and zsock_close() functions. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon force-pushed the net-sockets1 branch from f1b0460 to d8a31fb Compare June 26, 2017 15:22

pfalcon added 9 commits June 26, 2017 18:52

net: sockets: Implement bind(), connect(), listen(), accept()

f164d9f

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets: Add POSIX compat defines for inet_ntop, inet_pton

49f8888

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets: Implement send()

7c9b057

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets: Implement recv() for STREAM sockets

8958d1f

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets: Implement recv() for DGRAM sockets

9ddd43a

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets: Explicitly flush conn/pkt queue on close()

75d822d

If a socket is closed without reading all data from peer or accepting all pending connection, they will be leaked. So, flush queues explicitly. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

tests: net: Add test for Sockets API UDP socket/bind/connect/send/recv

c2a3d86

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon force-pushed the net-sockets1 branch from d8a31fb to c2a3d86 Compare June 26, 2017 15:52

jukkar approved these changes Jun 27, 2017

View reviewed changes

jukkar merged commit b650fa8 into zephyrproject-rtos:master Jun 28, 2017

This was referenced Sep 18, 2017

BSD Sockets API: Basic blocking API zephyriot/zep-jira14#2062

Closed

BSD Sockets API: Basic blocking API zephyriot/zephyr-issues#2062

Closed

zephyrbot mentioned this pull request Sep 23, 2017

BSD Sockets API: Basic blocking API #3662

Closed

zephyrbot mentioned this pull request Sep 16, 2019

arc: hsdk: bug fixes and new feature supports #19173

Merged

parthitce pushed a commit to linumiz/zephyr that referenced this pull request Jun 21, 2023

Issue zephyrproject-rtos#498: Fix a memory leak bug introduced in tes…

68abf12

…t_double_serializer in 485f2a0 by adding a json_object_put call.

parthitce pushed a commit to linumiz/zephyr that referenced this pull request Jun 21, 2023

.travis.yml:install valgrind

50e7fff

(zephyrproject-rtos#498)


		serv = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

		bind_addr.sin_family = AF_INET;

		CONFIG_NET_SLIP_TAP=y
		CONFIG_TEST_RANDOM_GENERATOR=y

net: BSD Sockets like API initial implementation #498

net: BSD Sockets like API initial implementation #498

Conversation

pfalcon commented Jun 14, 2017

pfalcon commented Jun 14, 2017

pfalcon commented Jun 15, 2017

pfalcon commented Jun 15, 2017

pfalcon commented Jun 15, 2017

jukkar 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

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

lpereira commented Jun 16, 2017

pfalcon commented Jun 20, 2017 • edited by zephyrbot Loading

pfalcon commented Jun 20, 2017

pfalcon commented Jun 20, 2017

pfalcon commented Jun 20, 2017

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

pfalcon commented Jun 22, 2017

pfalcon commented Jun 22, 2017 • edited Loading

pfalcon commented Jun 22, 2017

pfalcon commented Jun 26, 2017

pfalcon commented Jun 29, 2017

pfalcon commented Jun 20, 2017 •

edited by zephyrbot

Loading

pfalcon commented Jun 22, 2017 •

edited

Loading