Looks good to me and compiles correctly on OpenBSD.
Could it be included in the next 3.2 release (3.2.2)?
On 2/6/20 19:31, Ken Murchison wrote:
> Yes, you're correct. We have a comment in prot.c that quotes the same
> passage ion the docs.
>
> I think this might do the trick:
>
>
> diff --git a/imap/httpd.c b/imap/httpd.c
> index fc430d935..b5014b97e 100644
> --- a/imap/httpd.c
> +++ b/imap/httpd.c
> @@ -149,24 +149,12 @@ HIDDEN int zlib_compress(struct transaction_t
> *txn, unsigned flags,
> zstrm->next_in = (Bytef *) buf;
> zstrm->avail_in = len;
>
> - buf_ensure(&txn->zbuf, deflateBound(zstrm, zstrm->avail_in));
> buf_reset(&txn->zbuf);
>
> do {
> int zr;
>
> - if (!zstrm->avail_out) {
> - unsigned pending;
> -
> - zr = deflatePending(zstrm, &pending, Z_NULL);
> - if (zr != Z_OK) {
> - /* something went wrong */
> - syslog(LOG_ERR, "zlib deflate error: %d %s", zr,
> zstrm->msg);
> - return -1;
> - }
> -
> - buf_ensure(&txn->zbuf, pending);
> - }
> + buf_ensure(&txn->zbuf, deflateBound(zstrm, len));
>
> zstrm->next_out = (Bytef *) txn->zbuf.s + txn->zbuf.len;
> zstrm->avail_out = txn->zbuf.alloc - txn->zbuf.len;
>
>
> On 6/2/20 6:19 PM, Anatoli wrote:
>> Hi Ken,
>>
>> According to the docs [1]: If deflate returns Z_OK and with zero
>> avail_out, it must be called again after making room in the buffer
>> because there might be more output pending.
>>
>> On the other hand it says: Z_FINISH can be used in the first deflate
>> call after deflateInit if all the compression is to be done in a single
>> step. In order to complete in one call, avail_out must be at least the
>> value returned by deflateBound. Then deflate is guaranteed to return
>> Z_STREAM_END.
>>
>> But: Note that it is possible for the compressed size to be larger than
>> the value returned by deflateBound() if flush options other than
>> Z_FINISH or Z_NO_FLUSH are used.
>>
>>
>> At the beginning of zlib_compress() there's code that decides with which
>> flush option to call deflate().
>>
>> /* Only flush for static content or on last (zero-length) chunk */
>>
>> If it's possible to make flush to be always Z_FINISH, then I guess the
>> entire
>>
>> do { ... } while (!zstrm->avail_out);
>>
>> loop could be simplified to just:
>>
>> zstrm->next_out = (Bytef *) txn->zbuf.s + txn->zbuf.len;
>> zstrm->avail_out = txn->zbuf.alloc - txn->zbuf.len;
>>
>> zr = deflate(zstrm, flush);
>> if (zr != Z_STREAM_END) {
>> /* something went wrong */
>> syslog(LOG_ERR, "zlib deflate error: %d %s", zr, zstrm->msg);
>> return -1;
>> }
>>
>>
>> I changed the "if (zr)" condition as the only good value would be
>> Z_STREAM_END.
>>
>> But if the flush option can't be always Z_FINISH, then I believe the
>> loop should be kept with the checks for !zstrm->avail_out as it's
>> possible that there would be not enough buffer for deflate() to complete
>> in one call.
>>
>> Regards,
>> Anatoli
>>
>> [1] https://www.zlib.net/manual.html
>>
>>
>>
>> On 2/6/20 17:36, Ken Murchison wrote:
>>> Hi Anatoli,
>>>
>>> Thanks for the report. I'm not sure that we even need the
>>> deflatePending() call, since we use deflateBound() to create an
>>> appropriately-sized buffer to hold the entire compressed response body.
>>> Let me do some testing.
>>>
>>>
>>> On 6/2/20 3:48 AM, Anatoli wrote:
>>>> Cyrus developers,
>>>>
>>>> Is it possible to somehow rework the code in imap/httpd.c lines 158-169
>>>> in order to NOT use deflatePending as this func is not available on
>>>> OpenBSD? The zlib version there is 1.2.3 and deflatePending appeared in
>>>> 1.2.5 so the code doesn't compile with --enable-http (undefined symbol:
>>>> deflatePending). The packaged version disables http for now.
>>>>
>>>> Is it safe to reduce these lines:
>>>>
>>>> 158 if (!zstrm->avail_out) {
>>>> 159 unsigned pending;
>>>> 160
>>>> 161 zr = deflatePending(zstrm, &pending, Z_NULL);
>>>> 162 if (zr != Z_OK) {
>>>> 163 /* something went wrong */
>>>> 164 syslog(LOG_ERR, "zlib deflate error: %d %s", zr,
>>>> zstrm->msg);
>>>> 165 return -1;
>>>> 166 }
>>>> 167
>>>> 168 buf_ensure(&txn->zbuf, pending);
>>>> 169 }
>>>>
>>>>
>>>> to something like:
>>>>
>>>> 158 if (!zstrm->avail_out) {
>>>> 159 buf_ensure(&txn->zbuf, 256 * 1024);
>>>> 160 }
>>>>
>>>> If I understand it correctly, deflatePending in this case is only used
>>>> to get the needed buffer size and we could replace it with a hardcoded
>>>> buffer size (like in my example of 256K).
>>>>
>>>>
>>>> Thanks,
>>>> Anatoli
>
