Re: [PATCH v3 7/8] reftable: add code to facilitate consistency checks

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 24, 2025, 18:40 UTC
Message-ID: <CAOLa=ZQ641MncC9ACm9jfjx0WtQ+nK2shtyucQOxd08LDXDzAw@mail.gmail.com>
In-Reply-To: <aNOHqEq5qxXrOCX7@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 27 quoted lines

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

Yeah, that would be much better. will change.

Show 10 quoted lines

>> +	ptr = endptr;
>> +
>> +	if (strncmp(ptr, "-", 1))
>> +		return false;
>
> Better:
>
>     if (*ptr != '-')
>         return false;
>

I did use that below. I think I missed changing this, will do.

Show 23 quoted lines

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

Yeah, I missed this in the previous iteration, but realized while reading the spec that this could be possible.

Show 12 quoted lines

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

Fair enough, will move.

Show 34 quoted lines

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

I actually had a more structured code here, where the idea was:

- For each stack
  - Run stack level checks
  - For each table in stack
    - Run table level checks
    - For each block in table
      - Run block level checks
      - For each ref / log
        - Run ref / log level checks

But we move some of my tests to be runtime checks, leaving this as the only check remaining. We could still do the first level of what I mentioned above. The only reason I didn't was because we wanted to check all files in the stack dir. But I think this is much better, having unknown files in the reftable directory doesn't affect the repository in any way. So I would argue perhaps that we shouldn't even care about it.

Show 22 quoted lines

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

Leftover code, will cleanup.

Show 7 quoted lines

>> +	int err = 0;
>> +
>> +	if (stack == NULL)
>> +		goto out;
>
> Why should someone ever pass a `NULL` stack?
>

This should be safe to remove.

Show 19 quoted lines

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

Yeah, mostly the idea was to break things up into layers as I mentioned above. Let's make it simpler for now and we can make it nicer when we get around adding more checks.

Show 24 quoted lines

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

Will do.