From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 24 Sep 2025 05:54:48 GMT
Subject: Re: [PATCH v3 7/8] reftable: add code to facilitate consistency checks
Message-ID: <aNOHqEq5qxXrOCX7@pks.im>
In-Reply-To: <20250918-228-reftable-introduce-consistency-checks-v3-7-271af03eb34d@gmail.com>

On Thu, Sep 18, 2025 at 10:11:48AM +0200, Karthik Nayak wrote:
> diff --git a/reftable/fsck.c b/reftable/fsck.c
> new file mode 100644
> index 0000000000..785e4b43e8
> --- /dev/null
> +++ b/reftable/fsck.c
> @@ -0,0 +1,112 @@
> +#include "basics.h"
> +#include "reftable-fsck.h"
> +#include "stack.h"
> +
> +static bool valid_table_name(const char *name, uint64_t *min_update_index,
> +			     uint64_t *max_update_index)
> +{
> +	const char *ptr = name;
> +	char *endptr;
> +
> +	/* strtoull doesn't set errno on success */
> +	errno = 0;
> +
> +	*min_update_index = strtoull(ptr, &endptr, 16);
> +	if (errno == EINVAL)
> +		return false;

strtoull may also return ERANGE. In general, shouldn't we abort whenever
errno is non-zero here?

> +	ptr = endptr;
> +
> +	if (strncmp(ptr, "-", 1))
> +		return false;

Better:

    if (*ptr != '-')
        return false;

> +	ptr++;
> +
> +	*max_update_index = strtoull(ptr, &endptr, 16);
> +	if (errno == EINVAL)
> +		return false;
> +	ptr = endptr;
> +
> +	if (*ptr != '-')
> +		return false;
> +	ptr++;
> +
> +	strtoul(ptr, &endptr, 16);
> +	if (errno == EINVAL)
> +		return false;
> +	ptr = endptr;
> +
> +	if (strcmp(ptr, ".ref") && strcmp(ptr, ".log"))
> +		return false;

Yup, makes sense. We don't do so ourselves, but in theory it is possible
for tables to have a ".log" suffix. If so, they are expected to only
contain reflog records.

> +	return true;
> +}
> +
> +static int stack_check_all_files_in_dir(struct reftable_stack *stack,
> +					reftable_fsck_report_fn report_fn,
> +					void *cb_data)
> +{
> +	DIR *dir = opendir(stack->reftable_dir);

I think it would make sense to move this function call close to the
conditional.

> +	struct reftable_fsck_info info;
> +	struct dirent *d = NULL;
> +	uint64_t min, max;
> +	int err = 0;
> +
> +	if (!dir)
> +		return 0;
> +
> +	while ((d = readdir(dir))) {
> +		if (!strcmp(d->d_name, "tables.list"))
> +			continue;
> +
> +		if ((d->d_name[0] == '.' &&
> +		     (d->d_name[1] == '\0' ||
> +		      (d->d_name[1] == '.' && d->d_name[2] == '\0'))))
> +			continue;
> +
> +		if (d->d_type == DT_REG) {
> +			if (!valid_table_name(d->d_name, &min, &max)) {
> +				info.error = REFTABLE_FSCK_ERROR_TABLE_NAME;
> +				info.msg = "file with invalid table name";
> +				info.path = d->d_name;
> +
> +				err |= report_fn(&info, cb_data);
> +			}

One problem with this is that this is racy with concurrent writers. We
don't recognize the "tables.list.lock" file, and neither do we recognize
"0x*-0x*.{ref,log}.temp.XXXXXX"-style files.

Would it be a better approach be to instead go through table names as
loaded by the stack? The reftable code already knows to prune unknown
files anyway, so I don't think we should scan for any other files.

> +		} else {
> +			info.error = REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE;
> +			info.msg = "file with unexpected type";
> +			info.path = d->d_name;
> +
> +			err |= report_fn(&info, cb_data);
> +		}
> +	}
> +
> +	closedir(dir);
> +	return err;
> +}
> +
> +static int stack_checks(struct reftable_stack *stack,
> +			reftable_fsck_report_fn report_fn,
> +			void *cb_data)
> +{
> +	struct reftable_buf msg = REFTABLE_BUF_INIT;
> +	char **names = NULL;

This variable is unused.

> +	int err = 0;
> +
> +	if (stack == NULL)
> +		goto out;

Why should someone ever pass a `NULL` stack?

> +	err |= stack_check_all_files_in_dir(stack, report_fn, cb_data);
> +
> +out:
> +	free_names(names);
> +	reftable_buf_release(&msg);
> +	return err;
> +}
> +
> +int reftable_fsck_check(struct reftable_stack *stack,
> +			reftable_fsck_report_fn report_fn,
> +			reftable_fsck_verbose_fn verbose_fn,
> +			void *cb_data)
> +{
> +	verbose_fn("Checking reftable: stack checks", cb_data);
> +	return stack_checks(stack, report_fn, cb_data);

Nit: having this extra function call to `stack_checks()` feels a bit
weird as it could just as well be inlined. Is this preparing for a
future change?

> +}
> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
> new file mode 100644
> index 0000000000..5e13ac9f02
> --- /dev/null
> +++ b/reftable/reftable-fsck.h
> @@ -0,0 +1,42 @@
> +#ifndef REFTABLE_FSCK_H
> +#define REFTABLE_FSCK_H
> +
> +#include "reftable-stack.h"
> +
> +enum reftable_fsck_error {
> +	/* Non regular file in the reftable directory */
> +	REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE = 0,
> +	/* Invalid table name */
> +	REFTABLE_FSCK_ERROR_TABLE_NAME,
> +	/* Used for bounds checking, must be last */
> +	REFTABLE_FSCK_MAX_VALUE

Let's add a trailing comma here.

Patrick