Subject: constants for tar header offsets

From: Robert Haas <[email-masked]>
Date: Tue, 18 Apr 2023 11:20:06 -0400

Hi,

We have a few different places in the code where we generate or modify
tar headers or just read data out of them. The code in question uses
one of my less-favorite programming things: magic numbers. The offsets
of the various fields within the tar header are just hard-coded in
each relevant place in our code. I think we should clean that up, as
in the attached patch.

I hasten to emphasize that, while I think this is an improvement, I
don't think the result is particularly awesome. Even with the patch,
src/port/tar.c and src/include/pgtar.h do a poor job insulating
callers from the details of the tar format. However, it's also not
very clear to me how to fix that. For instance, I thought about
writing a function that parses a tar header into a struct and then
using it in all of these places, but that seems like it would lose too
much efficiency relative to the current ad-hoc coding. So for now I
don't have a better idea than this.

--
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Tue, 18 Apr 2023 11:38:56 -0400

> each relevant place in our code. I think we should clean that up, as

Generally +1, with a couple of additional thoughts:

1. Is it worth inventing macros for the values of the file type,
rather than just writing the comment you did?

2. The header size is defined as 512 bytes, but this doesn't sum to 512:

+	TAR_OFFSET_PREFIX = 345		/* 155 byte string */

Either that field's length is really 167 bytes, or there's some other
field you didn't document.  (It looks like you may have copied "155"
from an incorrect existing comment?)

> very clear to me how to fix that.

Yeah, this is adding greppability (a good thing!) but little more.
However, I'm not convinced it's worth doing more.  It's not like
this data structure will change anytime soon.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Tue, 18 Apr 2023 11:53:08 -0400

> rather than just writing the comment you did?

Might be.

> from an incorrect existing comment?)

According to my research, it is neither of those, e.g. see

https://www.subspacefield.org/~vax/tar_format.html
https://www.ibm.com/docs/en/zos/2.4.0?topic=formats-tar-format-tar-archives
https://wiki.osdev.org/USTAR

I think that what happened is that whoever designed the original tar
format decided on 512 byte blocks. And the header did not take up the
whole block. The USTAR format is an extension of the original format
which uses more of the block, but still not all of it.

> this data structure will change anytime soon.

Right. greppability is a major concern for me here, and also bug
surface. Right now, to use the functions in pgtar.h, you need to know
all the header offsets as well as the format and length of each header
field. This centralizes constants for the header offsets, and at least
provides some centralized documentation of the rest. It's not great,
though, because it seems like there's some risk of someone writing new
code and getting confused about whether the length of a certain field
is 8 or 12, for example. A thicker abstraction layer might be able to
avoid or minimize such risks better than what we have, but I don't
really know how to design it, whereas this seems like an obvious
improvement.

---

From: Tom Lane <[email-masked]>
Date: Tue, 18 Apr 2023 12:06:09 -0400

> which uses more of the block, but still not all of it.

Hmm, you're right: I checked the POSIX.1-2018 spec as well, and
it agrees that the prefix field is 155 bytes long.  Perhaps just
add another comment line indicating that 12 bytes remain unassigned?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Tue, 18 Apr 2023 12:24:11 -0400

> add another comment line indicating that 12 bytes remain unassigned?

OK. Here's v2, with that change and a few others.

---

From: Tom Lane <[email-masked]>
Date: Tue, 18 Apr 2023 12:38:48 -0400

> OK. Here's v2, with that change and a few others.

LGTM.

			regards, tom lane