Add and use symbolic constants for tar header offsets and file types.

Because symbolic constants in a header file are better than magic
constants embedded in the code.

Patch by me, reviewed by Tom Lane, Dagfinn Ilmari Mannsåker, and
Tristan Partin.

Discussion: http://postgr.es/m/CA+TgmoZNbLwhmCrNtkJAvi8FLkwFdMeVU3myV2HQQpA5bvbRZg@mail.gmail.com

diff --git a/src/bin/pg_basebackup/bbstreamer_tar.c b/src/bin/pg_basebackup/bbstreamer_tar.c
index 03d7fd3375a85b80305ee001e6a2b19d30a1ac93..d7b438d0f5920e6701cd06de0c2d3134297ba8f8 100644 (file)
--- a/src/bin/pg_basebackup/bbstreamer_tar.c
+++ b/src/bin/pg_basebackup/bbstreamer_tar.c
@@ -286,22 +286,20 @@ bbstreamer_tar_header(bbstreamer_tar_parser *mystreamer)
 
        /*
         * Parse key fields out of the header.
-        *
-        * FIXME: It's terrible that we use hard-coded values here instead of some
-        * more principled approach. It's been like this for a long time, but we
-        * ought to do better.
         */
-       strlcpy(member->pathname, &buffer[0], MAXPGPATH);
+       strlcpy(member->pathname, &buffer[TAR_OFFSET_NAME], MAXPGPATH);
        if (member->pathname[0] == '\0')
                pg_fatal("tar member has empty name");
-       member->size = read_tar_number(&buffer[124], 12);
-       member->mode = read_tar_number(&buffer[100], 8);
-       member->uid = read_tar_number(&buffer[108], 8);
-       member->gid = read_tar_number(&buffer[116], 8);
-       member->is_directory = (buffer[156] == '5');
-       member->is_link = (buffer[156] == '2');
+       member->size = read_tar_number(&buffer[TAR_OFFSET_SIZE], 12);
+       member->mode = read_tar_number(&buffer[TAR_OFFSET_MODE], 8);
+       member->uid = read_tar_number(&buffer[TAR_OFFSET_UID], 8);
+       member->gid = read_tar_number(&buffer[TAR_OFFSET_GID], 8);
+       member->is_directory =
+               (buffer[TAR_OFFSET_TYPEFLAG] == TAR_FILETYPE_DIRECTORY);
+       member->is_link =
+               (buffer[TAR_OFFSET_TYPEFLAG] == TAR_FILETYPE_SYMLINK);
        if (member->is_link)
-               strlcpy(member->linktarget, &buffer[157], 100);
+               strlcpy(member->linktarget, &buffer[TAR_OFFSET_LINKNAME], 100);
 
        /* Compute number of padding bytes. */
        mystreamer->pad_bytes_expected = tarPaddingBytesRequired(member->size);

diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 376ddf72b7104640cede8dd049cbb00ad5e0af28..d780c4055cd99d64d62b7ec3f7e786bc8e8f7534 100644 (file)
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -1131,7 +1131,7 @@ tar_close(Walfile *f, WalCloseMethod method)
         * possibly also renaming the file. We overwrite the entire current header
         * when done, including the checksum.
         */
-       print_tar_number(&(tf->header[124]), 12, filesize);
+       print_tar_number(&(tf->header[TAR_OFFSET_SIZE]), 12, filesize);
 
        if (method == CLOSE_NORMAL)
 
@@ -1139,9 +1139,10 @@ tar_close(Walfile *f, WalCloseMethod method)
                 * We overwrite it with what it was before if we have no tempname,
                 * since we're going to write the buffer anyway.
                 */
-               strlcpy(&(tf->header[0]), tf->base.pathname, 100);
+               strlcpy(&(tf->header[TAR_OFFSET_NAME]), tf->base.pathname, 100);
 
-       print_tar_number(&(tf->header[148]), 8, tarChecksum(((TarMethodFile *) f)->header));
+       print_tar_number(&(tf->header[TAR_OFFSET_CHECKSUM]), 8,
+                                        tarChecksum(((TarMethodFile *) f)->header));
        if (lseek(tar_data->fd, tf->ofs_start, SEEK_SET) != ((TarMethodFile *) f)->ofs_start)
        {
                f->wwmethod->lasterrno = errno;

diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index db5fb43bae8f783b7db29f8c76f2d041ff5d919b..aad88ad559fa8eada78f489860a04bbc70e44c95 100644 (file)
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -975,20 +975,20 @@ isValidTarHeader(char *header)
        int                     sum;
        int                     chk = tarChecksum(header);
 
-       sum = read_tar_number(&header[148], 8);
+       sum = read_tar_number(&header[TAR_OFFSET_CHECKSUM], 8);
 
        if (sum != chk)
                return false;
 
        /* POSIX tar format */
-       if (memcmp(&header[257], "ustar\0", 6) == 0 &&
-               memcmp(&header[263], "00", 2) == 0)
+       if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar\0", 6) == 0 &&
+               memcmp(&header[TAR_OFFSET_VERSION], "00", 2) == 0)
                return true;
        /* GNU tar format */
-       if (memcmp(&header[257], "ustar  \0", 8) == 0)
+       if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar  \0", 8) == 0)
                return true;
        /* not-quite-POSIX format written by pre-9.3 pg_dump */
-       if (memcmp(&header[257], "ustar00\0", 8) == 0)
+       if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar00\0", 8) == 0)
                return true;
 
        return false;
@@ -1151,7 +1151,7 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
 
                /* Calc checksum */
                chk = tarChecksum(h);
-               sum = read_tar_number(&h[148], 8);
+               sum = read_tar_number(&h[TAR_OFFSET_CHECKSUM], 8);
 
                /*
                 * If the checksum failed, see if it is a null block. If so, silently
@@ -1175,9 +1175,9 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
        }
 
        /* Name field is 100 bytes, might not be null-terminated */
-       strlcpy(tag, &h[0], 100 + 1);
+       strlcpy(tag, &h[TAR_OFFSET_NAME], 100 + 1);
 
-       len = read_tar_number(&h[124], 12);
+       len = read_tar_number(&h[TAR_OFFSET_SIZE], 12);
 
        pg_log_debug("TOC Entry %s at %llu (length %llu, checksum %d)",
                                 tag, (unsigned long long) hPos, (unsigned long long) len, sum);

diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index 661f9d7c59f104f16aab03cfe5a648f0e22bd134..8abfb9c19c305c5bf86b0429081585ab858fcd11 100644 (file)
--- a/src/include/pgtar.h
+++ b/src/include/pgtar.h
@@ -23,6 +23,45 @@ enum tarError
        TAR_SYMLINK_TOO_LONG
 };
 
+/*
+ * Offsets of fields within a 512-byte tar header.
+ *
+ * "tar number" values should be generated using print_tar_number() and can be
+ * read using read_tar_number(). Fields that contain strings are generally
+ * both filled and read using strlcpy().
+ *
+ * The value for the checksum field can be computed using tarChecksum().
+ *
+ * Some fields are not used by PostgreSQL; see tarCreateHeader().
+ */
+enum tarHeaderOffset
+{
+       TAR_OFFSET_NAME = 0,            /* 100 byte string */
+       TAR_OFFSET_MODE = 100,          /* 8 byte tar number, excludes S_IFMT */
+       TAR_OFFSET_UID = 108,           /* 8 byte tar number */
+       TAR_OFFSET_GID = 116,           /* 8 byte tar number */
+       TAR_OFFSET_SIZE = 124,          /* 8 byte tar number */
+       TAR_OFFSET_MTIME = 136,         /* 12 byte tar number */
+       TAR_OFFSET_CHECKSUM = 148,      /* 8 byte tar number */
+       TAR_OFFSET_TYPEFLAG = 156,      /* 1 byte file type, see TAR_FILETYPE_* */
+       TAR_OFFSET_LINKNAME = 157,      /* 100 byte string */
+       TAR_OFFSET_MAGIC = 257,         /* "ustar" with terminating zero byte */
+       TAR_OFFSET_VERSION = 263,       /* "00" */
+       TAR_OFFSET_UNAME = 265,         /* 32 byte string */
+       TAR_OFFSET_GNAME = 297,         /* 32 byte string */
+       TAR_OFFSET_DEVMAJOR = 329,      /* 8 byte tar number */
+       TAR_OFFSET_DEVMINOR = 337,      /* 8 byte tar number */
+       TAR_OFFSET_PREFIX = 345         /* 155 byte string */
+       /* last 12 bytes of the 512-byte block are unassigned */
+};
+
+enum tarFileType
+{
+       TAR_FILETYPE_PLAIN = '0',
+       TAR_FILETYPE_SYMLINK = '2',
+       TAR_FILETYPE_DIRECTORY = '5'
+};
+
 extern enum tarError tarCreateHeader(char *h, const char *filename,
                                                                         const char *linktarget, pgoff_t size,
                                                                         mode_t mode, uid_t uid, gid_t gid,

diff --git a/src/port/tar.c b/src/port/tar.c
index 4afe9f2533482b72b413ee81464fb4a7e577bb99..592b4fb7b0f4e08fb9c9efdee6439e54661aa600 100644 (file)
--- a/src/port/tar.c
+++ b/src/port/tar.c
@@ -120,10 +120,10 @@ tarCreateHeader(char *h, const char *filename, const char *linktarget,
        if (linktarget && strlen(linktarget) > 99)
                return TAR_SYMLINK_TOO_LONG;
 
-       memset(h, 0, 512);                      /* assume tar header size */
+       memset(h, 0, TAR_BLOCK_SIZE);
 
        /* Name 100 */
-       strlcpy(&h[0], filename, 100);
+       strlcpy(&h[TAR_OFFSET_NAME], filename, 100);
        if (linktarget != NULL || S_ISDIR(mode))
        {
                /*
@@ -139,68 +139,68 @@ tarCreateHeader(char *h, const char *filename, const char *linktarget,
        }
 
        /* Mode 8 - this doesn't include the file type bits (S_IFMT)  */
-       print_tar_number(&h[100], 8, (mode & 07777));
+       print_tar_number(&h[TAR_OFFSET_MODE], 8, (mode & 07777));
 
        /* User ID 8 */
-       print_tar_number(&h[108], 8, uid);
+       print_tar_number(&h[TAR_OFFSET_UID], 8, uid);
 
        /* Group 8 */
-       print_tar_number(&h[116], 8, gid);
+       print_tar_number(&h[TAR_OFFSET_GID], 8, gid);
 
        /* File size 12 */
        if (linktarget != NULL || S_ISDIR(mode))
                /* Symbolic link or directory has size zero */
-               print_tar_number(&h[124], 12, 0);
+               print_tar_number(&h[TAR_OFFSET_SIZE], 12, 0);
        else
-               print_tar_number(&h[124], 12, size);
+               print_tar_number(&h[TAR_OFFSET_SIZE], 12, size);
 
        /* Mod Time 12 */
-       print_tar_number(&h[136], 12, mtime);
+       print_tar_number(&h[TAR_OFFSET_MTIME], 12, mtime);
 
        /* Checksum 8 cannot be calculated until we've filled all other fields */
 
        if (linktarget != NULL)
        {
                /* Type - Symbolic link */
-               h[156] = '2';
+               h[TAR_OFFSET_TYPEFLAG] = TAR_FILETYPE_SYMLINK;
                /* Link Name 100 */
-               strlcpy(&h[157], linktarget, 100);
+               strlcpy(&h[TAR_OFFSET_LINKNAME], linktarget, 100);
        }
        else if (S_ISDIR(mode))
        {
                /* Type - directory */
-               h[156] = '5';
+               h[TAR_OFFSET_TYPEFLAG] = TAR_FILETYPE_DIRECTORY;
        }
        else
        {
                /* Type - regular file */
-               h[156] = '0';
+               h[TAR_OFFSET_TYPEFLAG] = TAR_FILETYPE_PLAIN;
        }
 
        /* Magic 6 */
-       strcpy(&h[257], "ustar");
+       strcpy(&h[TAR_OFFSET_MAGIC], "ustar");
 
        /* Version 2 */
-       memcpy(&h[263], "00", 2);
+       memcpy(&h[TAR_OFFSET_VERSION], "00", 2);
 
        /* User 32 */
        /* XXX: Do we need to care about setting correct username? */
-       strlcpy(&h[265], "postgres", 32);
+       strlcpy(&h[TAR_OFFSET_UNAME], "postgres", 32);
 
        /* Group 32 */
        /* XXX: Do we need to care about setting correct group name? */
-       strlcpy(&h[297], "postgres", 32);
+       strlcpy(&h[TAR_OFFSET_GNAME], "postgres", 32);
 
        /* Major Dev 8 */
-       print_tar_number(&h[329], 8, 0);
+       print_tar_number(&h[TAR_OFFSET_DEVMAJOR], 8, 0);
 
        /* Minor Dev 8 */
-       print_tar_number(&h[337], 8, 0);
+       print_tar_number(&h[TAR_OFFSET_DEVMINOR], 8, 0);
 
        /* Prefix 155 - not used, leave as nulls */
 
        /* Finally, compute and insert the checksum */
-       print_tar_number(&h[148], 8, tarChecksum(h));
+       print_tar_number(&h[TAR_OFFSET_CHECKSUM], 8, tarChecksum(h));
 
        return TAR_OK;
 }