On 03/19/2018 06:54 PM, Dr. David Alan Gilbert wrote:

+    return 0;
+exit:
+    compress_threads_load_cleanup();

I don't think this is safe; if inflateInit(..) fails in not-the-last
thread, compress_threads_load_cleanup() will try and destroy all the
mutex's and condition variables, even though they've not yet all been
_init'd.


That is exactly why we used 'opaque', please see more below...

However, other than that I think the patch is OK; a chat with Dan
Berrange has convinced me this probably doesn't affect the stream
format, so that's OK.

One thing I would like is a comment as to how the 'opaque' field is
being used; I don't think I quite understand what you're doing there.

The zlib.h file says that:
"     The opaque value provided by the application will be passed as the first
    parameter for calls of zalloc and zfree.  This can be useful for custom
    memory management.  The compression library attaches no meaning to the
    opaque value."
So we can use it to store our private data.

Here, we use it as a indicator which shows if the thread is properly init'd
or not. If inflateInit() is successful we will set it to non-NULL, otherwise
it is NULL, so that the cleanup path can figure out the first thread failed
to do inflateInit().

OK, so I think you just need to add a comment to explain that. Put it
above the 'if (!decomp_param[i].stream.opaque) {' in
compress_threads_load_cleanup  so it'll be easy to understand.

Yes, indeed, i will do.

Thanks!


Reply via email to