On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> When we compact the reftable stack we first acquire the lock for the
> "tables.list" file and then reload the stack to check that it is still
> up-to-date. This is done by calling `stack_uptodate()`, which knows to
> return zero in case the stack is up-to-date, a positive value if it is
> not and a negative error code on unexpected conditions.
So `stack_uptodate()` returns a negative value for error cases and a
positive value if the stack is out of date. `REFTABLE_OUTDATED_ERROR` is
really also an error, but it is special cased to differentiate it from
the others.
> We don't do proper error checking though, but instead we only check
> whether the returned error code is non-zero. If so, we simply bubble it
> up the calling stack, which means that callers may see an unexpected
> positive value.
>
> Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
> Handle this situation in `reftable_addition_commit()`, where we perform
> a best-effort auto-compaction.
>
> All other callsites of `stack_uptodate()` know to handle a positive
> return value and thus don't need to be fixed.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/stack.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index f77d7f58e8..effa2fc8cb 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
> return err;
> }
>
> -/* -1 = error
> - 0 = up to date
> - 1 = changed. */
> +/*
> + * Check whether the given stack is up-to-date with what we have in memory.
> + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
> + * otherwise.
> + */
> static int stack_uptodate(struct reftable_stack *st)
> {
> char **names = NULL;
> @@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)
> * control. It is possible that a concurrent writer is already
> * trying to compact parts of the stack, which would lead to a
> * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
> - * already. This is a benign error though, so we ignore it.
> + * already. Similarly, the stack may have been rewritten by a
> + * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
> + * Both of these errors are benign, so we simply ignore them.
> */
> err = reftable_stack_auto_compact(add->stack);
> - if (err < 0 && err != REFTABLE_LOCK_ERROR)
> + if (err < 0 && err != REFTABLE_LOCK_ERROR &&
> + err != REFTABLE_OUTDATED_ERROR)
> goto done;
> err = 0;
> }
> @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
> goto done;
> }
>
> + /*
> + * Check whether the stack is up-to-date. We unfortunately cannot
> + * handle the situation gracefully in case it's _not_ up-to-date
> + * because the range of tables that the user has requested us to
> + * compact may have been changed. So instead we abort.
> + *
> + * We could in theory improve the situation by having the caller not
> + * pass in a range, but instead the list of tables to compact. If so,
> + * we could check that relevant tables still exist. But for now it's
> + * good enough to just abort.
> + */
> err = stack_uptodate(st);
> - if (err)
> + if (err < 0)
> goto done;
> + if (err > 0) {
> + err = REFTABLE_OUTDATED_ERROR;
> + goto done;
> + }
I was thinking that maybe `stack_uptodate()` could maybe handle
returning the `REFTABLE_OUTDATED_ERROR` directly so we could avoid
having to map the error here. This could require callers to check for
`err == REFTABLE_OUTDATED_ERROR` instead of `err > 0`. Probably not a
big deal either way though.
Otherwise this looks good to me :)
-Justin
>
> /*
> * Lock all tables in the user-provided range. This is the slice of our
>
> --
> 2.50.1.723.g3e08bea96f.dirty
>
>
