Re: [PATCH v3 7/8] reftable: don't second-guess errors from flock interface

> 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@pks.im>
> ---
>  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)

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

> + * `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
>