---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38439/#review99380
---
-1
See my latest comment on PROTON-992 as to why I don't think this is a workable
approach, but even given that:
* Since you introduced a global in any case to tell whether you have already
initialised the Cyrus library, why bother with the new API? Just use the global
to tell you when to initialise or not - that's what the SSL code does.
* This code will never finalise the use of the Cyrus library and will cause a
memory leak.
* The global you introduce is not actually necessarily thread safe - so that
this code is not even gauranteed to fix the multiple initialisation problem you
want to fix. You really have to use an atomic or use locks to ensure that every
CPU sees the same value and changes in the same order - It's unlikely you will
see this race, and it's likely not even possible on some architectures, but you
may just have reduced the race window (albeit by orders of magnitude).
Having taken into account these points you will end up with my suggested fix in
that comment which is to use an atomic reference count to decide when to
initialise and finalise the Cyrus library. For good measure you should make the
same change to the OpenSSL code too, to ensure it doesn't have any
initialisation race.
- Andrew Stitcher
On Sept. 16, 2015, 6:26 p.m., michael goulish wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38439/
> ---
>
> (Updated Sept. 16, 2015, 6:26 p.m.)
>
>
> Review request for qpid, Alan Conway, Kenneth Giusti, and Ted Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> ---
>
> PROTON-992 : introduce a pn_init() fn, so we can initialize things like Cyrus
> SASL exactly-once.
> New file proton.h to hold declaration, because this doesn't seem to fit
> anywhere else.
> There is a flag in the sasl code to show whether this new interface has been
> used or not.
> If not -- allow code to function as before. Don't require current proton
> apps to change.
>
>
> Diffs
> -
>
> proton-c/include/proton/proton.h PRE-CREATION
> proton-c/src/sasl/cyrus_sasl.c 809bad5
> proton-c/src/sasl/none_sasl.c 674326f
> proton-c/src/sasl/sasl-internal.h b3f4c7f
> proton-c/src/util.c e2c6727
>
> Diff: https://reviews.apache.org/r/38439/diff/
>
>
> Testing
> ---
>
> ctest -VV to confirm that not using the new interface does not break anything.
>
> And I hacked a copy of dispatch router to make it call the new pn_init() from
> main -- and confirm that the cyrus sasl server init and client init fns are
> called exactly once.
>
>
> Thanks,
>
> michael goulish
>
>
