Improve type handling in pg_dump's compress file API

After 0da243fed0 got committed, we've received a report about a compiler
warning, related to the new LZ4File_gets() function:

  compress_lz4.c: In function 'LZ4File_gets':
  compress_lz4.c:492:19: warning: comparison of unsigned expression in
                                  '< 0' is always false [-Wtype-limits]
    492 |         if (dsize < 0)

The reason is very simple - dsize is declared as size_t, which is an
unsigned integer, and thus the check is pointless and we might fail to
notice an error in some cases (or fail in a strange way a bit later).

The warning could have been silenced by simply changing the type, but we
realized the API mostly assumes all the libraries use the same types and
report errors the same way (e.g. by returning 0 and/or negative value).

But we can't make this assumption - the gzip/lz4 libraries already
disagree on some of this, and even if they did a library added in the
future might not.

The right solution is to define what the API does, and translate the
library-specific behavior in consistent way (so that the internal errors
are not exposed to users of our compression API). So this adjusts the
data types in a couple places, so that we don't miss library errors, and
simplifies and unifies the error reporting to simply return true/false
(instead of e.g. size_t).

While at it, make sure LZ4File_open_write() does not clobber errno in
case open_func() fails.

Author: Georgios Kokolatos
Reported-by: Alexander Lakhin
Reviewed-by: Tomas Vondra, Justin Pryzby
Discussion: https://postgr.es/m/[email protected]

1 files changed, 29 insertions, 9 deletions

diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index cdb15951ea9..7c2f9b5668d 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -100,8 +100,10 @@ struct CompressFileHandle
 	 * Pass either 'path' or 'fd' depending on whether a file path or a file
 	 * descriptor is available. 'mode' can be one of 'r', 'rb', 'w', 'wb',
 	 * 'a', and 'ab'. Requires an already initialized CompressFileHandle.
+	 *
+	 * Returns true on success and false on error.
 	 */
-	int			(*open_func) (const char *path, int fd, const char *mode,
+	bool		(*open_func) (const char *path, int fd, const char *mode,
 							  CompressFileHandle *CFH);
 
 	/*
@@ -109,19 +111,27 @@ struct CompressFileHandle
 	 *
 	 * 'mode' can be one of 'w', 'wb', 'a', and 'ab'. Requires an already
 	 * initialized CompressFileHandle.
+	 *
+	 * Returns true on success and false on error.
 	 */
-	int			(*open_write_func) (const char *path, const char *mode,
+	bool		(*open_write_func) (const char *path, const char *mode,
 									CompressFileHandle *CFH);
 
 	/*
 	 * Read 'size' bytes of data from the file and store them into 'ptr'.
+	 * Optionally it will store the number of bytes read in 'rsize'.
+	 *
+	 * Returns true on success and throws an internal error otherwise.
 	 */
-	size_t		(*read_func) (void *ptr, size_t size, CompressFileHandle *CFH);
+	bool		(*read_func) (void *ptr, size_t size, size_t *rsize,
+							  CompressFileHandle *CFH);
 
 	/*
 	 * Write 'size' bytes of data into the file from 'ptr'.
+	 *
+	 * Returns true on success and false on error.
 	 */
-	size_t		(*write_func) (const void *ptr, size_t size,
+	bool		(*write_func) (const void *ptr, size_t size,
 							   struct CompressFileHandle *CFH);
 
 	/*
@@ -130,28 +140,38 @@ struct CompressFileHandle
 	 *
 	 * Stop if an EOF or a newline is found first. 's' is always null
 	 * terminated and contains the newline if it was found.
+	 *
+	 * Returns 's' on success, and NULL on error or when end of file occurs
+	 * while no characters have been read.
 	 */
 	char	   *(*gets_func) (char *s, int size, CompressFileHandle *CFH);
 
 	/*
 	 * Read the next character from the compress file handle as 'unsigned
 	 * char' cast into 'int'.
+	 *
+	 * Returns the character read on success and throws an internal error
+	 * otherwise. It treats EOF as error.
 	 */
 	int			(*getc_func) (CompressFileHandle *CFH);
 
 	/*
 	 * Test if EOF is reached in the compress file handle.
+	 *
+	 * Returns true if it is reached.
 	 */
-	int			(*eof_func) (CompressFileHandle *CFH);
+	bool		(*eof_func) (CompressFileHandle *CFH);
 
 	/*
 	 * Close an open file handle.
+	 *
+	 * Returns true on success and false on error.
 	 */
-	int			(*close_func) (CompressFileHandle *CFH);
+	bool		(*close_func) (CompressFileHandle *CFH);
 
 	/*
-	 * Get a pointer to a string that describes an error that occurred during a
-	 * compress file handle operation.
+	 * Get a pointer to a string that describes an error that occurred during
+	 * a compress file handle operation.
 	 */
 	const char *(*get_error_func) (CompressFileHandle *CFH);
 
@@ -178,5 +198,5 @@ extern CompressFileHandle *InitCompressFileHandle(const pg_compress_specificatio
  */
 extern CompressFileHandle *InitDiscoverCompressFileHandle(const char *path,
 														  const char *mode);
-extern int	EndCompressFileHandle(CompressFileHandle *CFH);
+extern bool EndCompressFileHandle(CompressFileHandle *CFH);
 #endif