Thread

improve astreamer_gzip.c comments

Robert Haas <robertmhaas@gmail.com> — 2024-08-13T13:39:53Z

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