Hi, Over on the security mailing list, Tom Lane expressed discontent about a few things related to astreamer_gzip.c. Here's a patch to improve the comments to try to address those concerns.
This ended up being discussed on -security because f80b09bac87d6b49f5dbb6131da5fbd9b9773c5c moved the code, leading Coverity to issue a new round of warnings about everything it doesn't like about these files. Scrutinizing those warnings, Tom believed that there might be a bug in astreamer_gzip_writer_new, because it's not too obvious who is supposed to close which file descriptor. There's an existing comment about the issue in astreamer_gzip_writer_finalize(), but it was too far away from the code he was looking at for him to see it right away, so add another comment pointing to it, using wording suggested by Tom. Tom also heavily criticized the header comment for astreamer_gzip_writer_new(). I don't really think there's any big problem, but I'm OK with his suggestion that we should instead duplicate the comment from astreamer_plain_writer_new(), so this patch does that. In response to further comments from Tom, I have also added a further paragraph to try to make it clear that callers need to be careful about how they use any FILE that they pass to this function. It doesn't matter for current usage, because it's only used by pg_basebackup when it's writing to stdout, and I don't anticipate that it will matter for future usage either, but it doesn't hurt to mention it, so here we go. I'm posting this here rather than on the -security thread so that others may comment if they wish. -- Robert Haas EDB: http://www.enterprisedb.com
astreamer-gzip-comments.patch
Description: Binary data
