freshlybakedca.ke / git
Git fork
fork atom

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>

authored by

Patrick Steinhardt and committed by
Junio C Hamano 8fd7a0eb 54d25de3

+12 -31
unified split
+8 -29
reftable/stack.c
··· 698 699 err = flock_acquire(&add->tables_list_lock, st->list_file, 700 st->opts.lock_timeout_ms); 701 - if (err < 0) { 702 - if (errno == EEXIST) { 703 - err = REFTABLE_LOCK_ERROR; 704 - } else { 705 - err = REFTABLE_IO_ERROR; 706 - } 707 goto done; 708 - } 709 if (st->opts.default_permissions) { 710 if (chmod(add->tables_list_lock.path, 711 st->opts.default_permissions) < 0) { ··· 1212 * which are part of the user-specified range. 1213 */ 1214 err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); 1215 - if (err < 0) { 1216 - if (errno == EEXIST) 1217 - err = REFTABLE_LOCK_ERROR; 1218 - else 1219 - err = REFTABLE_IO_ERROR; 1220 goto done; 1221 - } 1222 1223 /* 1224 * Check whether the stack is up-to-date. We unfortunately cannot ··· 1272 * tables, otherwise there would be nothing to compact. 1273 * In that case, we return a lock error to our caller. 1274 */ 1275 - if (errno == EEXIST && last - (i - 1) >= 2 && 1276 flags & STACK_COMPACT_RANGE_BEST_EFFORT) { 1277 err = 0; 1278 /* ··· 1284 */ 1285 first = (i - 1) + 1; 1286 break; 1287 - } else if (errno == EEXIST) { 1288 - err = REFTABLE_LOCK_ERROR; 1289 - goto done; 1290 - } else { 1291 - err = REFTABLE_IO_ERROR; 1292 - goto done; 1293 } 1294 } 1295 1296 /* ··· 1299 * of tables. 1300 */ 1301 err = flock_close(&table_locks[nlocks++]); 1302 - if (err < 0) { 1303 - err = REFTABLE_IO_ERROR; 1304 goto done; 1305 - } 1306 } 1307 1308 /* ··· 1334 * the new table. 1335 */ 1336 err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); 1337 - if (err < 0) { 1338 - if (errno == EEXIST) 1339 - err = REFTABLE_LOCK_ERROR; 1340 - else 1341 - err = REFTABLE_IO_ERROR; 1342 goto done; 1343 - } 1344 1345 if (st->opts.default_permissions) { 1346 if (chmod(tables_list_lock.path,
··· 698 699 err = flock_acquire(&add->tables_list_lock, st->list_file, 700 st->opts.lock_timeout_ms); 701 + if (err < 0) 702 goto done; 703 + 704 if (st->opts.default_permissions) { 705 if (chmod(add->tables_list_lock.path, 706 st->opts.default_permissions) < 0) { ··· 1207 * which are part of the user-specified range. 1208 */ 1209 err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); 1210 + if (err < 0) 1211 goto done; 1212 1213 /* 1214 * Check whether the stack is up-to-date. We unfortunately cannot ··· 1262 * tables, otherwise there would be nothing to compact. 1263 * In that case, we return a lock error to our caller. 1264 */ 1265 + if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 && 1266 flags & STACK_COMPACT_RANGE_BEST_EFFORT) { 1267 err = 0; 1268 /* ··· 1274 */ 1275 first = (i - 1) + 1; 1276 break; 1277 } 1278 + 1279 + goto done; 1280 } 1281 1282 /* ··· 1285 * of tables. 1286 */ 1287 err = flock_close(&table_locks[nlocks++]); 1288 + if (err < 0) 1289 goto done; 1290 } 1291 1292 /* ··· 1318 * the new table. 1319 */ 1320 err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms); 1321 + if (err < 0) 1322 goto done; 1323 1324 if (st->opts.default_permissions) { 1325 if (chmod(tables_list_lock.path,
+1 -1
reftable/system.c
··· 72 reftable_free(lockfile); 73 if (errno == EEXIST) 74 return REFTABLE_LOCK_ERROR; 75 - return -1; 76 } 77 78 l->fd = get_lock_file_fd(lockfile);
··· 72 reftable_free(lockfile); 73 if (errno == EEXIST) 74 return REFTABLE_LOCK_ERROR; 75 + return REFTABLE_IO_ERROR; 76 } 77 78 l->fd = get_lock_file_fd(lockfile);
+3 -1
reftable/system.h
··· 81 * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative 82 * we block indefinitely. 83 * 84 - * Retrun 0 on success, a reftable error code on error. 85 */ 86 int flock_acquire(struct reftable_flock *l, const char *target_path, 87 long timeout_ms);
··· 81 * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative 82 * we block indefinitely. 83 * 84 + * Retrun 0 on success, a reftable error code on error. Specifically, 85 + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already 86 + * locked. 87 */ 88 int flock_acquire(struct reftable_flock *l, const char *target_path, 89 long timeout_ms);