Thread safety for lhash documented.#4429
Conversation
doc/man3/OPENSSL_LH_COMPFUNC.pod
Outdated
There was a problem hiding this comment.
I sense certain ambiguity here. Thing is that while read lock is sufficient for OPENSSL_LH_stats itself, it effectively depends on write locks at retrieve time. And I fail to see this relation emphasized. Would something along following be more appropriate? "... under a read lock, unless accurate usage statistic is desired. In which case a write lock [for retrieve operations] should be used, for OPENSSL_LH_stats read lock suffices." Idea is to "break" it into two "causality" chains...
doc/man3/OPENSSL_LH_stats.pod
Outdated
There was a problem hiding this comment.
This actually emphasizes my point quite vividly. It gives impression that read lock at OPENSSL_LH_stats is sufficient for consistent output. In this case would it be appropriate to omit "or inconsistent" and simply refer to OPENSSL_LH_COMPFUNC(3), and even more specifically to NOTE section?
da8338c to
57cdfc6
Compare
|
The POD wording has been improved as per the suggestions. |
doc/man3/OPENSSL_LH_COMPFUNC.pod
Outdated
There was a problem hiding this comment.
I'm not exactly happy about "always". Even though we can't deny user looking at inaccurate statistics, we probably shouldn't encourage it. I mean I feel that last sentence effectively does the latter. But on the other hand it might as well happen that I'm the only one who gets that impression. In other words I'd omit "always", would anybody else? I also wonder what effect would "the" have in "For output of the usage statistics". Would it be interpreted rather as "for output of [abovementioned accurate] usage statistics"? If so, it would also be an improvement in my ears...
There was a problem hiding this comment.
Both suggestions incorporated and squashed.
doc/man3/OPENSSL_LH_COMPFUNC.pod
Outdated
There was a problem hiding this comment.
Thanks. Got it correct two lines above :(
|
Documentation changes done now. |
|
Seeking an OMC tick. |
doc/man3/OPENSSL_LH_stats.pod
Outdated
There was a problem hiding this comment.
Actually, there's a notation, so you could simply have See L<OPENSSL_LH_COMPFUNC(3)/NOTE> for more details ...
There was a problem hiding this comment.
... and for HTML output, that'll make for a link to the correct anchor as well.
There was a problem hiding this comment.
Fixed, thanks.
I've learned my thing for the day :)
indicate the level of locking required for various operations. Remove the lock and atomics from the lhash code. These we're not complete or adequate. Refer to openssl#4418 and openssl#4427 for details.
|
ping? |
kaduk
left a comment
There was a problem hiding this comment.
Thank you for putting this together.
indicate the level of locking required for various operations. Remove the lock and atomics from the lhash code. These we're not complete or adequate. Refer to #4418 and #4427 for details. Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> Reviewed-by: Andy Polyakov <[email protected]> (Merged from #4429)
|
Merged, thanks. |
Document that lhash isn't thread safe under any circumstances and indicate the level of locking required for various operations. Remove the lock and atomics from the lhash code. These we're not complete
or adequate.
This replaces both #4418 and #4427 which will be closed unmerged.