Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem: assert used for ZAP error handling aborts process. #2478

Merged
merged 2 commits into from
Mar 29, 2017

Conversation

evoskuil
Copy link
Contributor

See: #2476

I've followed the general existing pattern whereby when a failure results when sending a message the zmq_msg_t object is not closed. This looks like a leak to me but it's a more general question than I wanted to resolve in this pull request. Worst case we are temporarily trading a crash for a leak in the case of an error that apparently isn't very common.

@bluca
Copy link
Member

bluca commented Mar 29, 2017

Why is there a massive diff in src/curve_server.cpp ?
It looks like nothing changed, just whitespace, I assume it's the Windows editor, could you please fix that? It will wreak havoc of git blame and git bisect otherwise

@evoskuil
Copy link
Contributor Author

evoskuil commented Mar 29, 2017

Yes, it results from the interaction with git automatic CRLF\LF conversion. When a file has a mixture of LF and CRLF characters (which this one does) git does not convert it to CRLF. But then the IDE does convert it, git doesn't then reset it to all LF (because it wasn't initially converted due to being mixed). :/

@bluca
Copy link
Member

bluca commented Mar 29, 2017

Any chance you could fix it so that the commit doesn't change every line please? Git blame is a really important tool

errno = EPROTO;
// Temporary support for security debugging
puts ("CURVE I: cannot open client HELLO -- wrong server key?");
errno = EPROTO;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section had \r\n line endings. Converted to \n (and fixed style).

@evoskuil
Copy link
Contributor Author

All set, squashed commits.

@bluca bluca merged commit 9c6fb09 into zeromq:master Mar 29, 2017
@bluca
Copy link
Member

bluca commented Mar 29, 2017

Great, thanks!

@bluca
Copy link
Member

bluca commented Mar 29, 2017

Regarding closing a message on failure to send, usually an error is returned to the caller who then has to take care of it, right? (On the phone so not easy to check around)

@evoskuil
Copy link
Contributor Author

Regarding message closure on failure, I created #2481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants