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: return code of sodium_init() is not checked. #69

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

bkmit
Copy link

@bkmit bkmit commented Nov 3, 2015

There are two todo comments in curve_client.cpp and curve_server.cpp that suggest
checking the return code of sodium_init() call. sodium_init() returns -1 on error,
0 on success and 1 if it has been called before and is already initalized:
https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/core.c

There are two todo comments in curve_client.cpp and curve_server.cpp that suggest
checking the return code of sodium_init() call. sodium_init() returns -1 on error,
0 on success and 1 if it has been called before and is already initalized:
https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/core.c
jemc added a commit that referenced this pull request Nov 3, 2015
Problem: return code of sodium_init() is not checked.
@jemc jemc merged commit a75b754 into zeromq:master Nov 3, 2015
@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

Please do not make patches directly to stable releases, and @zeromq/libzmq-maintainers please do not merge these.

@bkmit kindly make this patch/pull request to libzmq, and then we (or you) can backport it to zeromq4-1 with a test case, as we ask in our process.

@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

I've reverted this change, and to avoid future mishaps, changed permissions to stop merging into zeromq4-a and zeromq4-x.

If we allow patches to flow directly to this repo then we lose them.

The process for changing these repos is to backport selected fixes from master, and each backport needs:

  • an issue in the issue tracker
  • a test case

That's covered by C4.1 here: http://rfc.zeromq.org/spec:22#toc8

No harm done here, and sorry for breaking the flow. @bkmit if there is some special reason this change shouldn't be done on master, let me know.

@bkmit
Copy link
Author

bkmit commented Nov 4, 2015

This patch was cherry picked from libzmq: c-rack/libzmq@479db21 merged long ago.
Sorry for unintentionally breaking proper workflow.
Can be this backported here in proper way for making stable 0MQ library compilable with lates sodium?

@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

Ah, I didn't realize it was a backport... checked the latest commits on libzmq and it wasn't there.

My apologies. You did it right except (a) please mention it's a backport and (b) specify the issue, and if there's none, please make one. The release notes for a stable release are a list of issues fixed, and every backport must have an issue.

I guess a PR number on libzmq does count as an issue.

@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

If you need it, I'll make a new stable release once this is backported. The test case is to confirm we've not broken the stable release. If you want to confirm that manually by testing it after the patch is applied, that's fine here.

@bkmit
Copy link
Author

bkmit commented Nov 4, 2015

I'm not member of stable maintainers team. Can I ask for more knowledgable people to backport this patch in proper way? And, yes, stable release is very welcome!

@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

OK, I've canceled my revert. Your patch is good. For this to be accepted into zeromq4-1 I need an issue. If you would please make an issue on libzmq, and indicate that the fix is in libzmq, and in zeromq4-1, then we're good to go.

@c-rack
Copy link
Member

c-rack commented Nov 4, 2015

There is already an issue in libzmq: zeromq/libzmq#1632

@bkmit
Copy link
Author

bkmit commented Nov 4, 2015

Look like @sthen already opened issue zeromq/libzmq#1632

@hintjens
Copy link
Member

hintjens commented Nov 4, 2015

OK, all good, thanks.

On Wed, Nov 4, 2015 at 10:00 AM, Constantin Rack notifications@github.com
wrote:

There is already an issue in libzmq: zeromq/libzmq#1632
zeromq/libzmq#1632


Reply to this email directly or view it on GitHub
#69 (comment).

@rodgert
Copy link
Contributor

rodgert commented Nov 4, 2015

With the latest 4.1.3 tarball and gcc5.2 this fails the build
[-Werror-unused-result]

On Wed, Nov 4, 2015 at 3:02 AM, Pieter Hintjens notifications@github.com
wrote:

OK, all good, thanks.

On Wed, Nov 4, 2015 at 10:00 AM, Constantin Rack <notifications@github.com

wrote:

There is already an issue in libzmq: zeromq/libzmq#1632
zeromq/libzmq#1632


Reply to this email directly or view it on GitHub
#69 (comment).


Reply to this email directly or view it on GitHub
#69 (comment).

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.

5 participants