Coverity whinged this morning about the following bit in
the new pg_combinebackup code:
644 unsigned rb;
645
646 /* Read the block from the correct source, except if
dry-run. */
647 rb = pg_pread(s->fd, buffer, BLCKSZ, offsetmap[i]);
648 if (rb != BLCKSZ)
649 {
>>> CID 1559912: Control flow issues (NO_EFFECT)
>>> This less-than-zero comparison of an unsigned value is never true. "rb
>>> < 0U".
650 if (rb < 0)
651 pg_fatal("could not read file \"%s\": %m",
s->filename);
It's dead right to complain of course. (I kind of think that the
majority of places where reconstruct.c is using "unsigned" variables
are poorly-thought-through; many of them look like they should be
size_t, and I suspect some other ones beside this one are flat wrong
or at least unnecessarily fragile. But I digress.)
While looking around for other places that might've made comparable
mistakes, I noted that we have places that are storing the result of
pg_pread[v]/pg_pwrite[v] into an "int" variable even though they are
passing a size_t count argument that there is no obvious reason to
believe must fit in int. This seems like trouble waiting to happen,
so I fixed some of these in the attached. The major remaining place
that I think we ought to change is the newly-minted
FileRead[V]/FileWrite[V] functions, which are declared to return int
but really should be returning ssize_t IMO. I didn't do that here
though.
We could go further by insisting that *all* uses of pg_pread/pg_pwrite
use ssize_t result variables. I think that's probably overkill --- in
the example above, which is only asking to write BLCKSZ worth of data,
surely an int is sufficient. But you could argue that allowing this
pattern at all creates risk of copy/paste errors.
Of course the real elephant in the room is that plain old read(2)
and write(2) also return ssize_t. I've not attempted to vet every
call of those, and I think it'd likely be a waste of effort, as
we're unlikely to ever try to shove more than INT_MAX worth of
data through them. But it's a bit harder to make that argument
for the iovec-based file APIs. I think we ought to try to keep
our uses of those functions clean on this point.
Thoughts?
regards, tom lane
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e9019156a..1264849883 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2280,7 +2280,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
char *from;
Size nbytes;
Size nleft;
- int written;
+ ssize_t written;
instr_time start;
/* OK to write the page(s) */
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 5ee9628422..84246739ae 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -117,8 +117,8 @@ static void perform_base_backup(basebackup_options *opt, bbsink *sink,
IncrementalBackupInfo *ib);
static void parse_basebackup_options(List *options, basebackup_options *opt);
static int compareWalFileNames(const ListCell *a, const ListCell *b);
-static int basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
- const char *filename, bool partial_read_ok);
+static ssize_t basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
+ const char *filename, bool partial_read_ok);
/* Was the backup currently in-progress initiated in recovery mode? */
static bool backup_started_in_recovery = false;
@@ -525,7 +525,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
{
char *walFileName = (char *) lfirst(lc);
int fd;
- size_t cnt;
+ ssize_t cnt;
pgoff_t len = 0;
snprintf(pathbuf, MAXPGPATH, XLOGDIR "/%s", walFileName);
@@ -2079,11 +2079,11 @@ convert_link_to_directory(const char *pathbuf, struct stat *statbuf)
*
* Returns the number of bytes read.
*/
-static int
+static ssize_t
basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
const char *filename, bool partial_read_ok)
{
- int rc;
+ ssize_t rc;
pgstat_report_wait_start(WAIT_EVENT_BASEBACKUP_READ);
rc = pg_pread(fd, buf, nbytes, offset);
@@ -2096,7 +2096,7 @@ basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
if (!partial_read_ok && rc > 0 && rc != nbytes)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not read file \"%s\": read %d of %zu",
+ errmsg("could not read file \"%s\": read %zd of %zu",
filename, rc, nbytes)));
return rc;
diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c
index 6decdd8934..874e6cd150 100644
--- a/src/bin/pg_combinebackup/reconstruct.c
+++ b/src/bin/pg_combinebackup/reconstruct.c
@@ -504,7 +504,7 @@ make_rfile(char *filename, bool missing_ok)
static void
read_bytes(rfile *rf, void *buffer, unsigned length)
{
- unsigned rb = read(rf->fd, buffer, length);
+ int rb = read(rf->fd, buffer, length);
if (rb != length)
{
@@ -512,7 +512,7 @@ read_bytes(rfile *rf, void *buffer, unsigned length)
pg_fatal("could not read file \"%s\": %m", rf->filename);
else
pg_fatal("could not read file \"%s\": read only %d of %d bytes",
- rf->filename, (int) rb, length);
+ rf->filename, rb, length);
}
}
@@ -614,7 +614,7 @@ write_reconstructed_file(char *input_filename,
{
uint8 buffer[BLCKSZ];
rfile *s = sourcemap[i];
- unsigned wb;
+ int wb;
/* Update accounting information. */
if (s == NULL)
@@ -641,7 +641,7 @@ write_reconstructed_file(char *input_filename,
}
else
{
- unsigned rb;
+ int rb;
/* Read the block from the correct source, except if dry-run. */
rb = pg_pread(s->fd, buffer, BLCKSZ, offsetmap[i]);
@@ -650,9 +650,9 @@ write_reconstructed_file(char *input_filename,
if (rb < 0)
pg_fatal("could not read file \"%s\": %m", s->filename);
else
- pg_fatal("could not read file \"%s\": read only %d of %d bytes at offset %u",
- s->filename, (int) rb, BLCKSZ,
- (unsigned) offsetmap[i]);
+ pg_fatal("could not read file \"%s\": read only %d of %d bytes at offset %llu",
+ s->filename, rb, BLCKSZ,
+ (unsigned long long) offsetmap[i]);
}
}
@@ -663,7 +663,7 @@ write_reconstructed_file(char *input_filename,
pg_fatal("could not write file \"%s\": %m", output_filename);
else
pg_fatal("could not write file \"%s\": wrote only %d of %d bytes",
- output_filename, (int) wb, BLCKSZ);
+ output_filename, wb, BLCKSZ);
}
/* Update the checksum computation. */
