Update Unix Domain Socket creation/handling #1852

Suudy · 2016-03-14T15:50:09Z

Updated handling of Unix Domain Sockets, make use of temporary directories, and cleanup afterward.

bluca · 2016-03-14T16:03:48Z

Hi,

Thanks for the contribution!

Could you please fix the formatting (indentation should be 4 spaces)?

Also one of the test is failing:

lt-test_term_endpoint: ../tests/test_term_endpoint.cpp:148: int main(): Assertion `rc == 0' failed.
/bin/bash: line 5: 35277 Aborted (core dumped) ${dir}$tst
FAIL: tests/test_term_endpoint

I suspect it's due to the changes in zmq::ipc_listener_t::set_address from return -1 to goto error;

bluca · 2016-03-14T16:07:51Z

src/ipc_listener.cpp

+    int rc;
+
+    if ( s != -1 ) {
+      zmq_assert (s != retired_fd);


retired_fd is -1, so this check doesn't make much sense. Why the added "if {}" block?

On Mon, Mar 14, 2016 at 9:08 AM, Luca Boccassi notifications@github.com wrote:

In src/ipc_listener.cpp:

@@ -208,9 +260,13 @@ int zmq::ipc_listener_t::set_address (const char *addr_)

int zmq::ipc_listener_t::close ()
{

zmq_assert (s != retired_fd);

int rc = ::close (s);

errno_assert (rc == 0);

int rc;

if ( s != -1 ) {

zmq_assert (s != retired_fd);

retired_fd is -1, so this check doesn't make much sense. Why the added "if {}" block?

I'm trying to handle the case where the directory for the socket
failed to be created (i.e. mkdtemp() failed), then s would remain -1.
This would cause the assertion error "s != retired_fd" and casue
::close to return an error with errno set to EBADF. Though I suppose
this is also the case if open_socket() failed as well. So it seems
that in the case of either mkdtemp() or open_socket() failing, we
don't want the assertion or ::close() failure here.

Pete

Yes, and that's why, I think, the code as-is did a return -1 instead of goto err if open_socket failed.

We need the assert to enforce that this API is not used improperly, IE: called on a socket that's already closed. Do you think it's possible to leave that in with your changes?

On Mon, Mar 14, 2016 at 10:51 AM, Luca Boccassi
notifications@github.com wrote:

Yes, and that's why, I think, the code as-is did a return -1 instead of goto err if open_socket failed.

We need the assert to enforce that this API is not used improperly, IE: called on a socket that's already closed. Do you think it's possible to leave that in with your changes?

I may need to rework it a bit. What I was trying to do was cleanup
the mkdtemp() on an error. I thought that re-using
ipc_listener_t::close() was the best way to do that.

I can revert ipc_listener_t::close(), and update
ipc_listener_t::set_address() to properly cleanup the directory if
open_socket() fails. I understand your API checks, and I'll fix
things on my end.

Are the prohibitions on the use of exceptions? I initially coded it
to throw an exception in create_wildcard_address(), but didn't see
handling of exceptions elsewhere. If exceptions are ok, I can rework
set_address() to make sure that there are no stray files or
directories upon failure pretty easily. Otherwise, the code may look
a bit ugly.

Pete

I may need to rework it a bit. What I was trying to do was cleanup
the mkdtemp() on an error. I thought that re-using
ipc_listener_t::close() was the best way to do that.

If you need more time, I can merge as-is for now, and then you can send another PR to fix it when it's ready. Or maybe it's easier for you to work on the same branch?

I can revert ipc_listener_t::close(), and update
ipc_listener_t::set_address() to properly cleanup the directory if
open_socket() fails. I understand your API checks, and I'll fix
things on my end.

Are the prohibitions on the use of exceptions? I initially coded it
to throw an exception in create_wildcard_address(), but didn't see
handling of exceptions elsewhere. If exceptions are ok, I can rework
set_address() to make sure that there are no stray files or
directories upon failure pretty easily. Otherwise, the code may look
a bit ugly.

I personally don't have any objections to exceptions, but it's true that they are not used anywhere else. @hintjens any objections/opinions on exceptions?

On Mon, Mar 14, 2016 at 11:10 AM, Luca Boccassi
notifications@github.com wrote:

If you need more time, I can merge as-is for now, and then you can send another PR to fix it when it's ready. Or maybe it's easier for you to work on the same branch?

I can wait until I get things into a shape that meets with your
approval. No point in checking in "sorta" code.

I personally don't have any objections to exceptions, but it's true that they are not used anywhere else. @hintjens any objections/opinions on exceptions?

Well, if they aren't commonly used, it probably isn't that helpful to
divert from the common style already in use. I'll rework it without
exceptions.

Pete

On Mon, Mar 14, 2016 at 11:10 AM, Luca Boccassi
notifications@github.com wrote:

If you need more time, I can merge as-is for now, and then you can send
another PR to fix it when it's ready. Or maybe it's easier for you to work
on the same branch?

Actually, the cleanup was far easier than I thought. I reverted the
goto's back to returning -1, but also cleaning up any created
directory. I also reverted the API checks at the beginning of
ipc_listener_t::close() to the prior checks. I also added code to
preserve errno when trying to cleanup in these cases.

I also have the formatting cleaned up, and tweaked my code to have
opening braces on the same line (to match the rest of the file).

Pete

Awesome, thank you very much for taking the time to tweak the PR!

Suudy · 2016-03-14T16:49:50Z

On Mon, Mar 14, 2016 at 9:04 AM, Luca Boccassi notifications@github.com wrote:

Could you please fix the formatting (indentation should be 4 spaces)?

I've fixed that on my branch. I'll send a new pull request once I've
cleaned up the test failure and after we've discussed the change in
ipc_listener_t::close().

Also one of the test is failing:

lt-test_term_endpoint: ../tests/test_term_endpoint.cpp:148: int main():
Assertion `rc == 0' failed.
/bin/bash: line 5: 35277 Aborted (core dumped) ${dir}$tst
FAIL: tests/test_term_endpoint

I suspect it's due to the changes in zmq::ipc_listener_t::set_address from
return -1 to goto error;

Hmm.... Looking at the test, this shouldn't be the case. My change
should still return -1, but only after the close. Let me dig into it.

Pete

bluca · 2016-03-14T16:58:26Z

Thanks for looking into it!

In case you're not too familiar with Github, you can amend your commit and push -f to the same branch, and this PR will get updated automatically.

Suudy · 2016-03-14T17:47:35Z

On Mon, Mar 14, 2016 at 9:04 AM, Luca Boccassi notifications@github.com wrote:

lt-test_term_endpoint: ../tests/test_term_endpoint.cpp:148: int main(): Assertion `rc == 0' failed.
/bin/bash: line 5: 35277 Aborted (core dumped) ${dir}$tst
FAIL: tests/test_term_endpoint

This failure is multifaceted. The root cause of the failure is that
zmq_getsockopt() modifies the optvallen parameter, and it is note
restored before each call. The second problem is that the variable
passed to the zmq_getsockopt() is declared 'const size_t' even though
it is passed as non-const (which then modifies the value).

I think the reason they were passing before and began failing with my
commit was due to the fact that the AF_UNIX sockets are now longer in
length. And the previouly modified optvallen was large enough. My
fix restores buf_size to the actual buffer size before each call to
zmq_getsockopt().

I've fixed test_term_endpoint. However, I have two XFAIL's that I
can't quite figure out what is going on. They are in
test_req_correlate and test_req_relaxed. And looking at the code for
those, they seem completely unrelated to any of the changes I've made.
I reverted all my changes, and these same tests fail.

Suudy · 2016-03-14T17:49:07Z

On Mon, Mar 14, 2016 at 9:58 AM, Luca Boccassi notifications@github.com wrote:

In case you're not too familiar with Github, you can amend your commit and push -f to the same branch, and this PR will get updated automatically.

I'm not git expert, but I did manage two squash my commits into a
single commit and pushed them up to github. It looks like the pull
request has the proper changes.

Pete

bluca · 2016-03-14T17:58:34Z

On 14 March 2016 at 17:47, Suudy notifications@github.com wrote:

On Mon, Mar 14, 2016 at 9:04 AM, Luca Boccassi notifications@github.com
wrote:

lt-test_term_endpoint: ../tests/test_term_endpoint.cpp:148: int main():
Assertion `rc == 0' failed.
/bin/bash: line 5: 35277 Aborted (core dumped) ${dir}$tst
FAIL: tests/test_term_endpoint

This failure is multifaceted. The root cause of the failure is that
zmq_getsockopt() modifies the optvallen parameter, and it is note
restored before each call. The second problem is that the variable
passed to the zmq_getsockopt() is declared 'const size_t' even though
it is passed as non-const (which then modifies the value).

I think the reason they were passing before and began failing with my
commit was due to the fact that the AF_UNIX sockets are now longer in
length. And the previouly modified optvallen was large enough. My
fix restores buf_size to the actual buffer size before each call to
zmq_getsockopt().

It does seem a bit forced, yes. Thanks for taking care of it.

I've fixed test_term_endpoint. However, I have two XFAIL's that I
can't quite figure out what is going on. They are in
test_req_correlate and test_req_relaxed. And looking at the code for
those, they seem completely unrelated to any of the changes I've made.
I reverted all my changes, and these same tests fail.

Yes you can ignore those 2: XFAIL in autotools means it's marked as an
expected failure. We marked them as such a while ago, sorry for the
confusion!

…ories, and cleanup afterward. Fix test_term_endpoint handling of optvallen

Update Unix Domain Socket creation/handling

bluca · 2016-03-14T21:57:59Z

@Suudy - sorry to ping you again, but the same test is still failing on OSX:

FAIL: tests/test_term_endpoint Assertion failed: (rc == 0), function main, file ../tests/test_term_endpoint.cpp, line 152.

https://travis-ci.org/zeromq/libzmq/jobs/115954957

Any idea what could be the problem?

bluca · 2016-03-14T23:08:09Z

Never mind, it was the path buffer size, which becomes very long on OSX. Fix via: #1853

bluca · 2016-03-16T21:47:02Z

FYI: this is what the TMP path looks like on OSX:

ipc:///var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/tmphCFoJH/socket

Suudy · 2016-03-16T23:10:33Z

Out of curiosity, is TMP, TMPDIR, or TEMPDIR defined in the environment?

On Wed, Mar 16, 2016 at 2:47 PM, Luca Boccassi notifications@github.com
wrote:

FYI: this is what the TMP path looks like on OSX:

ipc:///var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/tmphCFoJH/socket

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1852 (comment)

bluca · 2016-03-17T21:44:45Z

TMPDIR is set to: /var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/
The other 2 are not set.

See log: https://travis-ci.org/bluca/libzmq/builds/116755306

Suudy · 2016-03-17T21:46:47Z

Ah. I can back out the environment check and use the current working
directory as it did prior if the long names are problematic or undesirable.

On Thursday, March 17, 2016, Luca Boccassi notifications@github.com wrote:

TMPDIR is set to: /var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/
The other 2 are not set.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1852 (comment)

bluca · 2016-03-17T21:51:39Z

No need to, I think it's fine as long as we know it can happen and the test has a big enough buffer, which it now has.

Suudy · 2016-03-17T21:55:10Z

It might be better to use FILENAME_MAX (from cstdio) for the buffer
sizing. I can generate a pull request with that fix for the test.

On Thu, Mar 17, 2016 at 2:51 PM, Luca Boccassi notifications@github.com
wrote:

No need to, I think it's fine as long as we know it can happen and the
test has a big enough buffer, which it now has.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1852 (comment)

bluca · 2016-03-17T22:00:55Z

Good idea, that's even better. Thanks!

Suudy · 2016-03-17T22:14:43Z

Done. Pull request #1857 has been created.

On Thu, Mar 17, 2016 at 3:01 PM, Luca Boccassi notifications@github.com
wrote:

Good idea, that's even better. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1852 (comment)

Suudy changed the title ~~Added files via upload~~ Update Unix Domain Socket creation/handling Mar 14, 2016

bluca reviewed Mar 14, 2016
View reviewed changes

Updated handling of Unix Domain Sockets, make use of temporary direct…

b6080a7

…ories, and cleanup afterward. Fix test_term_endpoint handling of optvallen

bluca added a commit that referenced this pull request Mar 14, 2016

Merge pull request #1852 from Suudy/master

5ce6bc5

Update Unix Domain Socket creation/handling

bluca merged commit 5ce6bc5 into zeromq:master Mar 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Unix Domain Socket creation/handling #1852

Update Unix Domain Socket creation/handling #1852

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca Mar 14, 2016

Suudy Mar 14, 2016

bluca Mar 14, 2016

Suudy Mar 14, 2016

bluca Mar 14, 2016

Suudy Mar 14, 2016

Suudy Mar 14, 2016

bluca Mar 14, 2016

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

Suudy commented Mar 14, 2016

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 16, 2016

Suudy commented Mar 16, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016

Update Unix Domain Socket creation/handling #1852

Update Unix Domain Socket creation/handling #1852

Conversation

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

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

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

Suudy commented Mar 14, 2016

Suudy commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 14, 2016

bluca commented Mar 16, 2016

Suudy commented Mar 16, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016

bluca commented Mar 17, 2016

Suudy commented Mar 17, 2016