Add method 'close' to RawSocket app

[dpino]

This PR solves issue #644

[eugeneia]

This should be RawSocket:stop. See the App API reference which lists the “special” methods: https://github.com/SnabbCo/snabbswitch/tree/v2015.10/src#app

[eugeneia]

Obvious bug, obvious fix. Or not? I would really like to see the following:

1. A reproducible test case that makes snabb crash due to leaked fd's
2. A fix that proves itself by passing the above test
3. The fix should include the regression test mentioned above in RawSocket's selftest

I imagine this PR would have the following commit messages:

1. Add “file descriptor leak test” to RawSocket:seltest (fails to pass CI)
2. Fix file descriptor leak in RawSocket (passes CI)

[lukego]

Passing thought:

I wonder if we could test many/all apps for this at the same time? Seems like any app that suffers from this class of error - leak of a scarce resource - could be tested by a loop that cycles back and forth between engine.configure() with an empty app network vs with an instance of the app under test.

This would seem to have caught the resource leaks in the intel10g driver that we once upon a time wrote tests for and fixed, and likewise the same resource likes in the draft intel1g driver that has no test coverage for this today.

T'would be quite awesome to have an app "stress tester" based on e.g. fuzzing or quickcheck.

[lukego]

... yet another thought:

The raw socket code should be rewritten in Lua based on ljsyscall. This would be shorter code and simpler because it doesn't cross programming language barriers. (It would also make the file descriptors automatically close when garbage collected - though I don't think we would want to depend on that.)

[dpino]

@eugeneia Thanks for the pointer :) With regard to testing, a loop that leaves the descriptors open until Snabb Switch sounds the way to go. I like the idea of the "stress tester" too.

I tested the patch manually. I run the selftest while monitoring file descriptors with strace:

sudo strace -e trace=open,close,read,write,connect,accept ./snabb snsh -t apps.socket.raw

write(4, "\0\0\0\0\0\2\0\0\0\0\0\1\206\335`\0\0\0\0\0;\1\0\0\0\0\0\0\0\0\0\0"..., 54) = 54
read(4, "\0\0\0\0\0\2\0\0\0\0\0\1\206\335`\0\0\0\0\0;\1\0\0\0\0\0\0\0\0\0\0"..., 10240) = 54
+++ exited with 0 +++

With the patch applied the last call is a close:

write(4, "\0\0\0\0\0\2\0\0\0\0\0\1\206\335`\0\0\0\0\0;\1\0\0\0\0\0\0\0\0\0\0"..., 54) = 54
read(4, "\0\0\0\0\0\2\0\0\0\0\0\1\206\335`\0\0\0\0\0;\1\0\0\0\0\0\0\0\0\0\0"..., 10240) = 54
close(4)                                = 0
+++ exited with 0 +++

I think I will go with @eugeneia suggestions first, and after try to generalize the code so it can be reusable for other apps.

[dpino]

Here's the output of the selftest:

$ sudo ./snabb snsh -t apps.socket.raw
open raw socket: Too many open files
Selftest passed

Once the system reports "Too many open files", the loop is exited and opened descriptors are closed.

[eugeneia]

Please fix 2ebd725, and revert 52fa53e and 19b8f95, then I think this PR is good to go. Sorry for the confusion.

[dpino]

Done. Now constructors return an error code in case objects could not be created. The test includes an assert that verifies creation of RawSocket object was fine.

I pushed 3 additional commits. I think it makes sense at least to squash the reverted patches.

[eugeneia]

Am I right in assuming this PR is superseded by #655?

[dpino]

@eugeneia That's correct.

[eugeneia]

Closing it then. 🔑