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

diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index d7b6f4a5280daae7bc02e275cbfd927a8db54dec..bf7feedf3462240e6d0f2c761e0a5c8d9319b62c 100644 (file)
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -7,23 +7,20 @@
  *
  * 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 "common/controldata_utils.h"
 #include "getopt_long.h"
+#include "pg_getopt.h"
 #include "storage/bufpage.h"
 #include "storage/checksum.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 blocks = 0;
@@ -36,7 +33,7 @@ static bool verbose = false;
 static const char *progname;
 
 static void
-usage()
+usage(void)
 {
    printf(_("%s verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
    printf(_("Usage:\n"));
@@ -52,7 +49,7 @@ usage()
    printf(_("Report bugs to <pgsql-bugs@postgresql.org>.\n"));
 }
 
-static const char *skip[] = {
+static const char *const skip[] = {
    "pg_control",
    "pg_filenode.map",
    "pg_internal.init",
@@ -61,9 +58,9 @@ static const char *skip[] = {
 };
 
 static bool
-skipfile(char *fn)
+skipfile(const char *fn)
 {
-   const char **f;
+   const char *const *f;
 
    if (strcmp(fn, ".") == 0 ||
        strcmp(fn, "..") == 0)
@@ -76,12 +73,12 @@ skipfile(char *fn)
 }
 
 static void
-scan_file(char *fn, int segmentno)
+scan_file(const char *fn, BlockNumber segmentno)
 {
    char        buf[BLCKSZ];
    PageHeader  header = (PageHeader) buf;
    int         f;
-   int         blockno;
+   BlockNumber blockno;
 
    f = open(fn, O_RDONLY | PG_BINARY);
    if (f < 0)
@@ -102,21 +99,21 @@ scan_file(char *fn, int segmentno)
            break;
        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);
            exit(1);
        }
        blocks++;
 
        /* New pages have no checksum yet */
-       if (PageIsNew(buf))
+       if (PageIsNew(header))
            continue;
 
        csum = pg_checksum_page(buf, blockno + segmentno * RELSEG_SIZE);
        if (csum != header->pd_checksum)
        {
            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);
            badblocks++;
        }
@@ -130,7 +127,7 @@ scan_file(char *fn, int segmentno)
 }
 
 static void
-scan_directory(char *basedir, char *subdir)
+scan_directory(const char *basedir, const char *subdir)
 {
    char        path[MAXPGPATH];
    DIR        *dir;
@@ -146,7 +143,7 @@ scan_directory(char *basedir, char *subdir)
    }
    while ((de = readdir(dir)) != NULL)
    {
-       char        fn[MAXPGPATH + 1];
+       char        fn[MAXPGPATH];
        struct stat st;
 
        if (skipfile(de->d_name))
@@ -161,9 +158,10 @@ scan_directory(char *basedir, char *subdir)
        }
        if (S_ISREG(st.st_mode))
        {
+           char        fnonly[MAXPGPATH];
            char       *forkpath,
                       *segmentpath;
-           int         segmentno = 0;
+           BlockNumber segmentno = 0;
 
            /*
             * 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
             * filtering.
             */
-           segmentpath = strchr(de->d_name, '.');
+           strlcpy(fnonly, de->d_name, sizeof(fnonly));
+           segmentpath = strchr(fnonly, '.');
            if (segmentpath != NULL)
            {
                *segmentpath++ = '\0';
@@ -184,11 +183,11 @@ scan_directory(char *basedir, char *subdir)
                }
            }
 
-           forkpath = strchr(de->d_name, '_');
+           forkpath = strchr(fnonly, '_');
            if (forkpath != NULL)
                *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 */
                continue;
 
@@ -247,7 +246,7 @@ main(int argc, char *argv[])
                DataDir = optarg;
                break;
            case 'r':
-               if (atoi(optarg) <= 0)
+               if (atoi(optarg) == 0)
                {
                    fprintf(stderr, _("%s: invalid relfilenode specification, must be numeric: %s\n"), progname, optarg);
                    exit(1);