On Mon, Jan 16, 2017 at 1:46 PM, Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Mon, Jan 16, 2017 at 9:12 PM, Magnus Hagander <mag...@hagander.net> > wrote: > > Where did your research point to then? :) I just read the gzip rfc > > (http://www.zlib.org/rfc-gzip.html) which seems to call it that at > least? > > Well, OK. I was not aware of this RFC. I guessed it by looking at the > code of gzip, that uses the CRC as well. I also found some reference > into a blog post. > Haha, ok. That was my first google hit, but I guess I luckily hit a better search keyword. I'll add a reference to the comment about it before commit. > >> > Finally, I think we should make the error message clearly say > >> > "compressed > >> > segment file" - just to make things extra clear. > >> > >> Sure. > > > > AFAICT the > > + iscompress ? "compressed" : "", > > part of the error handling is unnecessary, because iscompressed will > always > > be true in that block. All the other error messages in that codepath has > > compressed hardcoded in them, as should this one. > > Fat-fingered here.. > > >> Hm. It looks that you are right. zlib goes down to _tr_flush_bits() to > >> flush some output, but this finishes only with put_byte(). As the fd > >> is opaque in gzFile, it would be just better to open() the file first, > >> and then use gzdopen to get the gzFile. Let's use as well the existing > >> fd field to save it. gzclose() closes as well the parent fd per the > >> documentation of zlib. > > > > This version throws a warning: > > walmethods.c: In function ‘dir_open_for_write’: > > walmethods.c:170:11: warning: ‘gzfp’ may be used uninitialized in this > > function [-Wmaybe-uninitialized] > > f->gzfp = gzfp; > > gcc and clang did not complain here, what did you use? > gcc (Debian 4.9.2-10) 4.9.2 > I can't see that there is any code path where this can actually happen > > though, so we should probably just initialize it to NULL at variable > > declaration. Or do you see a path where this could actually be > incorrect? > > Not that I see. All the code paths using gzfp are under > data_dir->compression > 0. > > > If you agree with those two comments, I will go ahead and push with those > > minor fixes. > > No problem for me, thanks for the review! > Committed. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/