On 24/07/31 04:15PM, Patrick Steinhardt wrote:
> When modifying "tables.list", we need to lock the list before updating
> it to ensure that no concurrent writers modify the list at the same
> point in time. While we do this via the `lock_file` subsystem when
> compacting the stack, we manually handle the lock when adding a new
> table to it. While not wrong, it is at least inconsistent.
Indeed, much more consistent to use the lockfile API here. :)
>
> Refactor the code to consistenly lock "tables.list" via the `lock_file`
s/consistenly/consistently/
> subsytem.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/stack.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 9ca549294f..9cc91a262c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -567,7 +567,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
> }
>
> struct reftable_addition {
> - struct tempfile *lock_file;
> + struct lock_file tables_list_lock;
> struct reftable_stack *stack;
>
> char **new_tables;
> @@ -581,13 +581,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> struct reftable_stack *st)
> {
> struct strbuf lock_file_name = STRBUF_INIT;
> - int err = 0;
> - add->stack = st;
> + int err;
>
> - strbuf_addf(&lock_file_name, "%s.lock", st->list_file);
> + add->stack = st;
>
> - add->lock_file = create_tempfile(lock_file_name.buf);
> - if (!add->lock_file) {
> + err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,
> + LOCK_NO_DEREF);
> + if (err < 0) {
> if (errno == EEXIST) {
> err = REFTABLE_LOCK_ERROR;
> } else {
> @@ -596,7 +596,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> goto done;
> }
> if (st->opts.default_permissions) {
> - if (chmod(add->lock_file->filename.buf, st->opts.default_permissions) < 0) {
> + if (chmod(get_lock_file_path(&add->tables_list_lock),
> + st->opts.default_permissions) < 0) {
> err = REFTABLE_IO_ERROR;
> goto done;
> }
> @@ -635,7 +636,7 @@ static void reftable_addition_close(struct reftable_addition *add)
> add->new_tables_len = 0;
> add->new_tables_cap = 0;
>
> - delete_tempfile(&add->lock_file);
> + rollback_lock_file(&add->tables_list_lock);
> strbuf_release(&nm);
> }
>
> @@ -651,7 +652,7 @@ void reftable_addition_destroy(struct reftable_addition *add)
> int reftable_addition_commit(struct reftable_addition *add)
> {
> struct strbuf table_list = STRBUF_INIT;
> - int lock_file_fd = get_tempfile_fd(add->lock_file);
> + int lock_file_fd = get_lock_file_fd(&add->tables_list_lock);
> int err = 0;
> size_t i;
>
> @@ -674,13 +675,14 @@ int reftable_addition_commit(struct reftable_addition *add)
> goto done;
> }
>
> - err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);
> + err = fsync_component(FSYNC_COMPONENT_REFERENCE,
> + get_lock_file_fd(&add->tables_list_lock));
I might be missing something, but is there a reason we have to get the
lock file fd again instead of just using `lock_file_fd`?
> if (err < 0) {
> err = REFTABLE_IO_ERROR;
> goto done;
> }
>
> - err = rename_tempfile(&add->lock_file, add->stack->list_file);
> + err = commit_lock_file(&add->tables_list_lock);
> if (err < 0) {
> err = REFTABLE_IO_ERROR;
> goto done;
> --
> 2.46.0.dirty
>
