On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> The `flock` interface is implemented as part of "reftable/system.c" and
> thus needs to be implemented by the integrator between the reftable
> library and its parent code base. As such, we cannot rely on any
> specific implementation thereof.
>
> Regardless of that, users of the `flock` subsystem rely on `errno` being
> set to specific values. This is fragile and not documented anywhere and
> doesn't really make for a good interface.
>
> Refactor the code so that the implementations themselves are expected to
> return reftable-specific error codes. Our implementation of the `flock`
> subsystem already knows to do this for all error paths except one.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/stack.c | 37 ++++++++-----------------------------
> reftable/system.c | 2 +-
> reftable/system.h | 4 +++-
> 3 files changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index af0f94d882..f91ce50bcd 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>
> err = flock_acquire(&add->tables_list_lock, st->list_file,
> st->opts.lock_timeout_ms);
> - if (err < 0) {
> - if (errno == EEXIST) {
> - err = REFTABLE_LOCK_ERROR;
> - } else {
> - err = REFTABLE_IO_ERROR;
> - }
> + if (err < 0)
> goto done;
> - }
> +
> if (st->opts.default_permissions) {
> if (chmod(add->tables_list_lock.path,
> st->opts.default_permissions) < 0) {
> @@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,
> * which are part of the user-specified range.
> */
> err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
> - if (err < 0) {
> - if (errno == EEXIST)
> - err = REFTABLE_LOCK_ERROR;
> - else
> - err = REFTABLE_IO_ERROR;
> + if (err < 0)
> goto done;
> - }
>
> /*
> * Check whether the stack is up-to-date. We unfortunately cannot
> @@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,
> * tables, otherwise there would be nothing to compact.
> * In that case, we return a lock error to our caller.
> */
> - if (errno == EEXIST && last - (i - 1) >= 2 &&
> + if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&
> flags & STACK_COMPACT_RANGE_BEST_EFFORT) {
> err = 0;
> /*
> @@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,
> */
> first = (i - 1) + 1;
> break;
> - } else if (errno == EEXIST) {
> - err = REFTABLE_LOCK_ERROR;
> - goto done;
> - } else {
> - err = REFTABLE_IO_ERROR;
> - goto done;
> }
> +
> + goto done;
> }
>
> /*
> @@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,
> * of tables.
> */
> err = flock_close(&table_locks[nlocks++]);
> - if (err < 0) {
> - err = REFTABLE_IO_ERROR;
> + if (err < 0)
> goto done;
> - }
> }
>
> /*
> @@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,
> * the new table.
> */
> err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
> - if (err < 0) {
> - if (errno == EEXIST)
> - err = REFTABLE_LOCK_ERROR;
> - else
> - err = REFTABLE_IO_ERROR;
> + if (err < 0)
Now we no longer rely on errno to determine the correct err to return.
Nice.
> goto done;
> - }
>
> if (st->opts.default_permissions) {
> if (chmod(tables_list_lock.path,
> diff --git a/reftable/system.c b/reftable/system.c
> index 1ee268b125..725a25844e 100644
> --- a/reftable/system.c
> +++ b/reftable/system.c
> @@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,
> reftable_free(lockfile);
> if (errno == EEXIST)
> return REFTABLE_LOCK_ERROR;
> - return -1;
> + return REFTABLE_IO_ERROR;
> }
>
> l->fd = get_lock_file_fd(lockfile);
> diff --git a/reftable/system.h b/reftable/system.h
> index beb9d2431f..c54ed4cad6 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -81,7 +81,9 @@ struct reftable_flock {
> * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative
> * we block indefinitely.
> *
> - * Retrun 0 on success, a reftable error code on error.
> + * Retrun 0 on success, a reftable error code on error. Specifically,
Not a new typo, but we could fix it:
s/Retrun/Return/
> + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already
> + * locked.
> */
> int flock_acquire(struct reftable_flock *l, const char *target_path,
> long timeout_ms);
>
> --
> 2.51.0.rc1.163.g2494970778.dirty
>
