public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: constants for tar header offsets
Date: Tue, 18 Apr 2023 11:38:56 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoZNbLwhmCrNtkJAvi8FLkwFdMeVU3myV2HQQpA5bvbRZg@mail.gmail.com>
References: <CA+TgmoZNbLwhmCrNtkJAvi8FLkwFdMeVU3myV2HQQpA5bvbRZg@mail.gmail.com>

Robert Haas <[email protected]> writes:
> 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.

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?)

> 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.

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

view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: constants for tar header offsets
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox