Re: [PATCH 7/9] reftable/block: reuse zstream when writing log blocks

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -139,39 +143,60 @@ int block_writer_finish(struct block_writer *w)
> >  	w->next += 2;
> >  	put_be24(w->buf + 1 + w->header_off, w->next);
> >  
> > +	/*
> > +	 * Log records are stored zlib-compressed. Note that the compression
> > +	 * also spans over the restart points we have just written.
> > +	 */
> >  	if (block_writer_type(w) == BLOCK_TYPE_LOG) {
> >  		int block_header_skip = 4 + w->header_off;
> > +		uLongf src_len = w->next - block_header_skip, compressed_len;
> > +		unsigned char *compressed;
> > +		int ret;
> > +
> > +		ret = deflateReset(w->zstream);
> > +		if (ret != Z_OK)
> > +			return REFTABLE_ZLIB_ERROR;
> > +
> > +		/*
> > +		 * Precompute the upper bound of how many bytes the compressed
> > +		 * data may end up with. Combined with `Z_FINISH`, `deflate()`
> > +		 * is guaranteed to return `Z_STREAM_END`.
> > +		 */
> > +		compressed_len = deflateBound(w->zstream, src_len);
> > +		REFTABLE_ALLOC_ARRAY(compressed, compressed_len);
> 
> OK.
> 
> > +		w->zstream->next_out = compressed;
> > +		w->zstream->avail_out = compressed_len;
> > +		w->zstream->next_in = w->buf + block_header_skip;
> > +		w->zstream->avail_in = src_len;
> > +
> > +		/*
> > +		 * We want to perform all decompression in a single
> > +		 * step, which is why we can pass Z_FINISH here. Note
> > +		 * that both `Z_OK` and `Z_BUF_ERROR` indicate that we
> > +		 * need to retry according to documentation.
> > +		 *
> > +		 * If the call fails we retry with a bigger output
> > +		 * buffer.
> > +		 */
> 
> I am not sure where the retry is happening, though.
> 
> block_writer_finish() is called by writer_flush_nonempty_block()
> which returns a negative return to its caller, which is
> writer_flush_block().  writer_flush_block() in turn returns a
> negative return to its callers from writer_add_record(),
> write_finish_section(), and write_object_record().  Nobody seems to
> react to REFTABLE_ZLIB_ERROR (other than the reftable/error.c that
> stringifies the error for messages).
> 
> But we have asked deflateBound() so if we did not get Z_STREAM_END,
> wouldn't it mean some data corruption that retrying would not help?

> > +		ret = deflate(w->zstream, Z_FINISH);
> > +		if (ret != Z_STREAM_END) {
> >  			reftable_free(compressed);
> > -			break;
> > +			return REFTABLE_ZLIB_ERROR;
> >  		}
> > +
> > +		/*
> > +		 * Overwrite the uncompressed data we have already written and
> > +		 * adjust the `next` pointer to point right after the
> > +		 * compressed data.
> > +		 */
> > +		memcpy(w->buf + block_header_skip, compressed,
> > +		       w->zstream->total_out);
> > +		w->next = w->zstream->total_out + block_header_skip;
> > +
> > +		reftable_free(compressed);
> >  	}
> > +
> >  	return w->next;
> >  }
> 
> OK.
> 
> > @@ -425,6 +450,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
> >  
> >  void block_writer_release(struct block_writer *bw)
> >  {
> > +	deflateEnd(bw->zstream);
> > +	FREE_AND_NULL(bw->zstream);
> >  	FREE_AND_NULL(bw->restarts);
> >  	strbuf_release(&bw->last_key);
> >  	/* the block is not owned. */
> > diff --git a/reftable/block.h b/reftable/block.h
> > index 47acc62c0a..1375957fc8 100644
> > --- a/reftable/block.h
> > +++ b/reftable/block.h
> > @@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at
> >   * allocation overhead.
> >   */
> >  struct block_writer {
> > +	z_stream *zstream;
> >  	uint8_t *buf;
> >  	uint32_t block_size;
> >  
> > diff --git a/reftable/writer.c b/reftable/writer.c
> > index d347ec4cc6..51e663bb19 100644
> > --- a/reftable/writer.c
> > +++ b/reftable/writer.c
> > @@ -153,6 +153,10 @@ void reftable_writer_free(struct reftable_writer *w)
> >  {
> >  	if (!w)
> >  		return;
> > +	if (w->block_writer) {
> > +		block_writer_release(w->block_writer);
> > +		w->block_writer = NULL;
> > +	}
> 
> This smells like an orthogonal fix to an unrelated resource leakage?

> >  	reftable_free(w->block);
> >  	reftable_free(w);
> >  }
> 
> Thanks.