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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/stack.c b/reftable/stack.c
index af0f94d882644323622d4924572799362b9d0694..f91ce50bcdd4ee61d20617165b3db006ba32694f 100644 (file)
--- 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 1ee268b125ddb62d4bbec415d03b9c144c776d0c..725a25844ea1797995d2f5f787ef32248dcbb0a9 100644 (file)
--- 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 beb9d2431f7037b8c2a2c2ce40695f1aae60a9f4..c54ed4cad61f734b36c276097579f94cc649beaa 100644 (file)
--- 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);