Re: [PATCH v1] strengthen backup history filename check

On Mon, Jul 25, 2022 at 7:39 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > This patch makes the backup history filename check more tight.
>
> Can you please elaborate a bit on the issue with existing
> IsBackupHistoryFileName(), if there's any?
>

There are two call of this function, `CleanupBackupHistory` and
`SetWALFileNameForCleanup`, there
seems no issue of the existing IsBackupHistoryFileName() since the
creation of the backup history file
will make sure it is of format "%08X%08X%08X.%08X.backup".

The patch just makes `IsBackupHistoryFileName()` more match to
`BackupHistoryFileName()`, thus
more easier to understand.

> Also, the patch does have hard coded numbers [1] which isn't good from
> a readability perspective, adding macros and/or comments would help
> here.

I'm not sure using macros is a good idea here, cause I noticed
`IsTLHistoryFileName()` also uses
some hard code numbers [1].

[1]
static inline bool
IsTLHistoryFileName(const char *fname)
{
return (strlen(fname) == 8 + strlen(".history") &&
strspn(fname, "0123456789ABCDEF") == 8 &&
strcmp(fname + 8, ".history") == 0);
}

>
> [1]
> static inline bool
> IsBackupHistoryFileName(const char *fname)
> {
> - return (strlen(fname) > XLOG_FNAME_LEN &&
> + return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") &&
> strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
> - strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
> + strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 &&
> + strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0);
> }
>
> Regards,
> Bharath Rupireddy.

--
Regards
Junwang Zhao