Skip to content

Commit d3b5775

Browse files
tvondratvondra
committed
Improve type handling in pg_dump's compress file API
After 0da243f 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/33496f7c-3449-1426-d568-63f6bca2ac1f@gmail.com
1 parent a326aac commit d3b5775

File tree

7 files changed

+148
-115
lines changed

7 files changed

+148
-115
lines changed

‎src/bin/pg_dump/compress_gzip.c

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -233,14 +233,14 @@ InitCompressorGzip(CompressorState *cs,
233233
*----------------------
234234
*/
235235

236-
static size_t
237-
Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH)
236+
static bool
237+
Gzip_read(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH)
238238
{
239239
gzFile gzfp = (gzFile) CFH->private_data;
240-
size_t ret;
240+
int gzret;
241241

242-
ret = gzread(gzfp, ptr, size);
243-
if (ret != size && !gzeof(gzfp))
242+
gzret = gzread(gzfp, ptr, size);
243+
if (gzret <= 0 && !gzeof(gzfp))
244244
{
245245
int errnum;
246246
const char *errmsg = gzerror(gzfp, &errnum);
@@ -249,15 +249,18 @@ Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH)
249249
errnum == Z_ERRNO ? strerror(errno) : errmsg);
250250
}
251251

252-
return ret;
252+
if (rsize)
253+
*rsize = (size_t) gzret;
254+
255+
return true;
253256
}
254257

255-
static size_t
258+
static bool
256259
Gzip_write(const void *ptr, size_t size, CompressFileHandle *CFH)
257260
{
258261
gzFile gzfp = (gzFile) CFH->private_data;
259262

260-
return gzwrite(gzfp, ptr, size);
263+
return gzwrite(gzfp, ptr, size) > 0;
261264
}
262265

263266
static int
@@ -287,22 +290,22 @@ Gzip_gets(char *ptr, int size, CompressFileHandle *CFH)
287290
return gzgets(gzfp, ptr, size);
288291
}
289292

290-
static int
293+
static bool
291294
Gzip_close(CompressFileHandle *CFH)
292295
{
293296
gzFile gzfp = (gzFile) CFH->private_data;
294297

295298
CFH->private_data = NULL;
296299

297-
return gzclose(gzfp);
300+
return gzclose(gzfp) == Z_OK;
298301
}
299302

300-
static int
303+
static bool
301304
Gzip_eof(CompressFileHandle *CFH)
302305
{
303306
gzFile gzfp = (gzFile) CFH->private_data;
304307

305-
return gzeof(gzfp);
308+
return gzeof(gzfp) == 1;
306309
}
307310

308311
static const char *
@@ -319,7 +322,7 @@ Gzip_get_error(CompressFileHandle *CFH)
319322
return errmsg;
320323
}
321324

322-
static int
325+
static bool
323326
Gzip_open(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
324327
{
325328
gzFile gzfp;
@@ -342,18 +345,18 @@ Gzip_open(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
342345
gzfp = gzopen(path, mode_compression);
343346

344347
if (gzfp == NULL)
345-
return 1;
348+
return false;
346349

347350
CFH->private_data = gzfp;
348351

349-
return 0;
352+
return true;
350353
}
351354

352-
static int
355+
static bool
353356
Gzip_open_write(const char *path, const char *mode, CompressFileHandle *CFH)
354357
{
355358
char *fname;
356-
int ret;
359+
bool ret;
357360
int save_errno;
358361

359362
fname = psprintf("%s.gz", path);

‎src/bin/pg_dump/compress_io.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ InitDiscoverCompressFileHandle(const char *path, const char *mode)
262262
}
263263

264264
CFH = InitCompressFileHandle(compression_spec);
265-
if (CFH->open_func(fname, -1, mode, CFH))
265+
if (!CFH->open_func(fname, -1, mode, CFH))
266266
{
267267
free_keep_errno(CFH);
268268
CFH = NULL;
@@ -275,12 +275,12 @@ InitDiscoverCompressFileHandle(const char *path, const char *mode)
275275
/*
276276
* Close an open file handle and release its memory.
277277
*
278-
* On failure, returns an error value and sets errno appropriately.
278+
* On failure, returns false and sets errno appropriately.
279279
*/
280-
int
280+
bool
281281
EndCompressFileHandle(CompressFileHandle *CFH)
282282
{
283-
int ret = 0;
283+
bool ret = false;
284284

285285
if (CFH->private_data)
286286
ret = CFH->close_func(CFH);

‎src/bin/pg_dump/compress_io.h

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -100,28 +100,38 @@ struct CompressFileHandle
100100
* Pass either 'path' or 'fd' depending on whether a file path or a file
101101
* descriptor is available. 'mode' can be one of 'r', 'rb', 'w', 'wb',
102102
* 'a', and 'ab'. Requires an already initialized CompressFileHandle.
103+
*
104+
* Returns true on success and false on error.
103105
*/
104-
int (*open_func) (const char *path, int fd, const char *mode,
106+
bool (*open_func) (const char *path, int fd, const char *mode,
105107
CompressFileHandle *CFH);
106108

107109
/*
108110
* Open a file for writing.
109111
*
110112
* 'mode' can be one of 'w', 'wb', 'a', and 'ab'. Requires an already
111113
* initialized CompressFileHandle.
114+
*
115+
* Returns true on success and false on error.
112116
*/
113-
int (*open_write_func) (const char *path, const char *mode,
117+
bool (*open_write_func) (const char *path, const char *mode,
114118
CompressFileHandle *CFH);
115119

116120
/*
117121
* Read 'size' bytes of data from the file and store them into 'ptr'.
122+
* Optionally it will store the number of bytes read in 'rsize'.
123+
*
124+
* Returns true on success and throws an internal error otherwise.
118125
*/
119-
size_t (*read_func) (void *ptr, size_t size, CompressFileHandle *CFH);
126+
bool (*read_func) (void *ptr, size_t size, size_t *rsize,
127+
CompressFileHandle *CFH);
120128

121129
/*
122130
* Write 'size' bytes of data into the file from 'ptr'.
131+
*
132+
* Returns true on success and false on error.
123133
*/
124-
size_t (*write_func) (const void *ptr, size_t size,
134+
bool (*write_func) (const void *ptr, size_t size,
125135
struct CompressFileHandle *CFH);
126136

127137
/*
@@ -130,28 +140,38 @@ struct CompressFileHandle
130140
*
131141
* Stop if an EOF or a newline is found first. 's' is always null
132142
* terminated and contains the newline if it was found.
143+
*
144+
* Returns 's' on success, and NULL on error or when end of file occurs
145+
* while no characters have been read.
133146
*/
134147
char *(*gets_func) (char *s, int size, CompressFileHandle *CFH);
135148

136149
/*
137150
* Read the next character from the compress file handle as 'unsigned
138151
* char' cast into 'int'.
152+
*
153+
* Returns the character read on success and throws an internal error
154+
* otherwise. It treats EOF as error.
139155
*/
140156
int (*getc_func) (CompressFileHandle *CFH);
141157

142158
/*
143159
* Test if EOF is reached in the compress file handle.
160+
*
161+
* Returns true if it is reached.
144162
*/
145-
int (*eof_func) (CompressFileHandle *CFH);
163+
bool (*eof_func) (CompressFileHandle *CFH);
146164

147165
/*
148166
* Close an open file handle.
167+
*
168+
* Returns true on success and false on error.
149169
*/
150-
int (*close_func) (CompressFileHandle *CFH);
170+
bool (*close_func) (CompressFileHandle *CFH);
151171

152172
/*
153-
* Get a pointer to a string that describes an error that occurred during a
154-
* compress file handle operation.
173+
* Get a pointer to a string that describes an error that occurred during
174+
* a compress file handle operation.
155175
*/
156176
const char *(*get_error_func) (CompressFileHandle *CFH);
157177

@@ -178,5 +198,5 @@ extern CompressFileHandle *InitCompressFileHandle(const pg_compress_specificatio
178198
*/
179199
extern CompressFileHandle *InitDiscoverCompressFileHandle(const char *path,
180200
const char *mode);
181-
extern int EndCompressFileHandle(CompressFileHandle *CFH);
201+
extern bool EndCompressFileHandle(CompressFileHandle *CFH);
182202
#endif

0 commit comments

Comments
 (0)