rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Code review for pg_verify_checksums.c. 

Use postgres_fe.h, since this is frontend code.  Pretend that we've heard
of project style guidelines for, eg, #include order.  Use BlockNumber not
int arithmetic for block numbers, to avoid misbehavior with relations
exceeding 2^31 blocks.  Avoid an unnecessary strict-aliasing warning
(per report from Michael Banck).  Const-ify assorted stuff.  Avoid
scribbling on the output of readdir() -- perhaps that's safe in practice,
but POSIX forbids it, and this code has so far earned exactly zero
credibility portability-wise.  Editorialize on an ambiguously-worded
message.

I did not touch the problem of the "buf" local variable being possibly
insufficiently aligned; that's not specific to this code, and seems like
it should be fixed as part of a different, larger patch.

Discussion: https://postgr.es/m/1535618100.1286.3.camel@credativ.de
This commit is contained in:
Tom Lane 2018-08-31 13:42:18 -04:00
parent f919c165eb
commit d9c366f9e8
1 changed files with 23 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
src/bin/pg_verify_checksums/pg_verify_checksums.c
View File

@ -7,23 +7,20 @@
* *
* src/bin/pg_verify_checksums/pg_verify_checksums.c * src/bin/pg_verify_checksums/pg_verify_checksums.c
*/ */
#include "postgres_fe.h"
#define FRONTEND 1 #include <dirent.h>
#include <sys/stat.h>
#include <unistd.h>
#include "postgres.h"
#include "catalog/pg_control.h" #include "catalog/pg_control.h"
#include "common/controldata_utils.h" #include "common/controldata_utils.h"
#include "getopt_long.h" #include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h" #include "storage/bufpage.h"
#include "storage/checksum.h" #include "storage/checksum.h"
#include "storage/checksum_impl.h" #include "storage/checksum_impl.h"
#include <sys/stat.h>
#include <dirent.h>
#include <unistd.h>
#include "pg_getopt.h"
static int64 files = 0; static int64 files = 0;
static int64 blocks = 0; static int64 blocks = 0;
@ -36,7 +33,7 @@ static bool verbose = false;
static const char *progname; static const char *progname;
static void static void
usage() usage(void)
{ {
printf(_("%s verifies data checksums in a PostgreSQL database cluster.\n\n"), progname); printf(_("%s verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
printf(_("Usage:\n")); printf(_("Usage:\n"));
@ -52,7 +49,7 @@ usage()
printf(_("Report bugs to <pgsql-bugs@postgresql.org>.\n")); printf(_("Report bugs to <pgsql-bugs@postgresql.org>.\n"));
} }
static const char *skip[] = { static const char *const skip[] = {
"pg_control", "pg_control",
"pg_filenode.map", "pg_filenode.map",
"pg_internal.init", "pg_internal.init",
@ -61,9 +58,9 @@ static const char *skip[] = {
}; };
static bool static bool
skipfile(char *fn) skipfile(const char *fn)
{ {
const char **f; const char *const *f;
if (strcmp(fn, ".") == 0 || if (strcmp(fn, ".") == 0 ||
strcmp(fn, "..") == 0) strcmp(fn, "..") == 0)
@ -76,12 +73,12 @@ skipfile(char *fn)
} }
static void static void
scan_file(char *fn, int segmentno) scan_file(const char *fn, BlockNumber segmentno)
{ {
char buf[BLCKSZ]; char buf[BLCKSZ];
PageHeader header = (PageHeader) buf; PageHeader header = (PageHeader) buf;
int f; int f;
int blockno; BlockNumber blockno;
f = open(fn, O_RDONLY | PG_BINARY); f = open(fn, O_RDONLY | PG_BINARY);
if (f < 0) if (f < 0)
@ -102,21 +99,21 @@ scan_file(char *fn, int segmentno)
break; break;
if (r != BLCKSZ) if (r != BLCKSZ)
{ {
fprintf(stderr, _("%s: short read of block %d in file \"%s\", got only %d bytes\n"), fprintf(stderr, _("%s: short read of block %u in file \"%s\", got only %d bytes\n"),
progname, blockno, fn, r); progname, blockno, fn, r);
exit(1); exit(1);
} }
blocks++; blocks++;
/* New pages have no checksum yet */ /* New pages have no checksum yet */
if (PageIsNew(buf)) if (PageIsNew(header))
continue; continue;
csum = pg_checksum_page(buf, blockno + segmentno * RELSEG_SIZE); csum = pg_checksum_page(buf, blockno + segmentno * RELSEG_SIZE);
if (csum != header->pd_checksum) if (csum != header->pd_checksum)
{ {
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION) if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
fprintf(stderr, _("%s: checksum verification failed in file \"%s\", block %d: calculated checksum %X but expected %X\n"), fprintf(stderr, _("%s: checksum verification failed in file \"%s\", block %u: calculated checksum %X but block contains %X\n"),
progname, fn, blockno, csum, header->pd_checksum); progname, fn, blockno, csum, header->pd_checksum);
badblocks++; badblocks++;
} }
@ -130,7 +127,7 @@ scan_file(char *fn, int segmentno)
} }
static void static void
scan_directory(char *basedir, char *subdir) scan_directory(const char *basedir, const char *subdir)
{ {
char path[MAXPGPATH]; char path[MAXPGPATH];
DIR *dir; DIR *dir;
@ -146,7 +143,7 @@ scan_directory(char *basedir, char *subdir)
} }
while ((de = readdir(dir)) != NULL) while ((de = readdir(dir)) != NULL)
{ {
char fn[MAXPGPATH + 1]; char fn[MAXPGPATH];
struct stat st; struct stat st;
if (skipfile(de->d_name)) if (skipfile(de->d_name))
@ -161,9 +158,10 @@ scan_directory(char *basedir, char *subdir)
} }
if (S_ISREG(st.st_mode)) if (S_ISREG(st.st_mode))
{ {
char fnonly[MAXPGPATH];
char *forkpath, char *forkpath,
*segmentpath; *segmentpath;
int segmentno = 0; BlockNumber segmentno = 0;
/* /*
* Cut off at the segment boundary (".") to get the segment number * Cut off at the segment boundary (".") to get the segment number
@ -171,7 +169,8 @@ scan_directory(char *basedir, char *subdir)
* fork boundary, to get the relfilenode the file belongs to for * fork boundary, to get the relfilenode the file belongs to for
* filtering. * filtering.
*/ */
segmentpath = strchr(de->d_name, '.'); strlcpy(fnonly, de->d_name, sizeof(fnonly));
segmentpath = strchr(fnonly, '.');
if (segmentpath != NULL) if (segmentpath != NULL)
{ {
*segmentpath++ = '\0'; *segmentpath++ = '\0';
@ -184,11 +183,11 @@ scan_directory(char *basedir, char *subdir)
} }
} }
forkpath = strchr(de->d_name, '_'); forkpath = strchr(fnonly, '_');
if (forkpath != NULL) if (forkpath != NULL)
*forkpath++ = '\0'; *forkpath++ = '\0';
if (only_relfilenode && strcmp(only_relfilenode, de->d_name) != 0) if (only_relfilenode && strcmp(only_relfilenode, fnonly) != 0)
/* Relfilenode not to be included */ /* Relfilenode not to be included */
continue; continue;
@ -247,7 +246,7 @@ main(int argc, char *argv[])
DataDir = optarg; DataDir = optarg;
break; break;
case 'r': case 'r':
if (atoi(optarg) <= 0) if (atoi(optarg) == 0)
{ {
fprintf(stderr, _("%s: invalid relfilenode specification, must be numeric: %s\n"), progname, optarg); fprintf(stderr, _("%s: invalid relfilenode specification, must be numeric: %s\n"), progname, optarg);
exit(1); exit(1);