> On Nov. 6, 2016, 5:47 p.m., Anand Mazumdar wrote: > > hmm, I couldn't find any documentation around when `msg` may be not be set > > for error cases except for `deflateEnd()` and `inflateEnd()`. Can you share > > some? > > AFAICT, we only need to add a fallback for these 2 calls and not all of the > > ones as has been done in this review. Also, I don't think we need to even > > check the return type of `deflateEnd()`/`inflateEnd()` since we are sure > > that the state of the stream is consistent i.e., `Z_STREAM_END`?
Per our offline discussion, it appears that zlib does not set `msg` in many cases and other libraries work around this, e.g. https://github.com/python/cpython/blob/c30098c8c6014f3340a369a31df9c74bdbacc269/Modules/zlibmodule.c#L59-L86 I've updated this patch to introduce an `internal::GzipError` to simply the error handling logic. I've also updated to `ABORT` for any programming errors, that is (in|de)flateInit2 / (in|de)flateEnd failures, since these have to be run inside the Decompressor and Compressor (in the future) constructors and destructors. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53365/#review155091 ----------------------------------------------------------- On Nov. 8, 2016, 4:17 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53365/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2016, 4:17 a.m.) > > > Review request for mesos and Anand Mazumdar. > > > Repository: mesos > > > Description > ------- > > It turns out that zlib does not always set the `msg` field, the > calling code is expected to handle the case where `msg` is NULL. > I discovered this while I was playing with the library during > the implementation of a streaming decompressor. > > I've introduced an `internal::GzipError` to simplify the error > handling, and I've updated the code to `ABORT` in the case of > our own programming errors. > > > Diffs > ----- > > 3rdparty/stout/include/stout/gzip.hpp > b78a8a31204ee585f8e4a88eaefe7346faa46b8d > > Diff: https://reviews.apache.org/r/53365/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
