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

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->size = read_tar_number(&buffer[TAR_OFFSET_SIZE], 12);
-	member->mode = read_tar_number(&buffer[100], 8);
+	member->mode = read_tar_number(&buffer[TAR_OFFSET_MODE], 8);
-	member->uid = read_tar_number(&buffer[108], 8);
+	member->uid = read_tar_number(&buffer[TAR_OFFSET_UID], 8);
-	member->gid = read_tar_number(&buffer[116], 8);
+	member->gid = read_tar_number(&buffer[TAR_OFFSET_GID], 8);
-	member->is_directory = (buffer[156] == '5');
+	member->is_directory =
-	member->is_link = (buffer[156] == '2');
+		(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);

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;

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 &&
+	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar\0", 6) == 0 &&
-		memcmp(&header[263], "00", 2) == 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);

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,

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;
 }