From 568d7229bf18983afba99776f847898bc1fe8677 Mon Sep 17 00:00:00 2001 From: Alasdair Kergon Date: Fri, 15 Mar 2002 16:07:38 +0000 Subject: [PATCH] Review locking: block signals instead of ignoring them and restore state afterwards; avoid race condition with unlink; add LCK_HOLD to process_each_vg. --- lib/locking/file_locking.c | 109 ++++++++++++++++++++++++++----------- lib/locking/locking.c | 60 +++++++++++--------- tools/toollib.c | 4 +- tools/vgcfgbackup.c | 2 +- tools/vgchange.c | 2 +- tools/vgck.c | 2 +- tools/vgdisplay.c | 2 +- tools/vgexport.c | 2 +- tools/vgimport.c | 2 +- tools/vgremove.c | 3 +- tools/vgscan.c | 2 +- 11 files changed, 121 insertions(+), 69 deletions(-) diff --git a/lib/locking/file_locking.c b/lib/locking/file_locking.c index 4ec614f62..1e98d1e91 100644 --- a/lib/locking/file_locking.c +++ b/lib/locking/file_locking.c @@ -32,11 +32,17 @@ struct lock_list { static struct list _lock_list; static char _lock_dir[NAME_LEN]; +static sig_t _oldhandler; +static sigset_t _fullsigset, _intsigset; +static int _handler_installed; + static int _release_lock(const char *file) { struct lock_list *ll; struct list *llh, *llt; + struct stat buf1, buf2; + list_iterate_safe(llh, llt, &_lock_list) { ll = list_item(llh, struct lock_list); @@ -44,10 +50,13 @@ static int _release_lock(const char *file) list_del(llh); log_very_verbose("Unlocking %s", ll->res); - /* - * If this is the last pid using the file, remove it - */ - if (!flock(ll->lf, LOCK_NB | LOCK_EX)) + if (flock(ll->lf, LOCK_NB | LOCK_UN)) + log_sys_error("flock", ll->res); + + if (!flock(ll->lf, LOCK_NB | LOCK_EX) && + !stat(ll->res, &buf1) && + !fstat(ll->lf, &buf2) && + !memcmp(&buf1.st_ino, &buf2.st_ino, sizeof(ino_t))) if (unlink(ll->res)) log_sys_error("unlink", ll->res); @@ -70,22 +79,35 @@ void fin_file_locking(void) _release_lock(NULL); } +static void _remove_ctrl_c_handler() +{ + siginterrupt(SIGINT, 0); + if (!_handler_installed || _oldhandler == SIG_ERR) + return; + + sigprocmask(SIG_SETMASK, &_fullsigset, NULL); + if (signal(SIGINT, _oldhandler) == SIG_ERR) + log_sys_error("signal", "_remove_ctrl_c_handler"); + + _handler_installed = 0; +} + void _trap_ctrl_c(int signal) { + _remove_ctrl_c_handler(); log_error("CTRL-c detected: giving up waiting for lock"); return; } static void _install_ctrl_c_handler() { - siginterrupt(SIGINT, 1); - signal(SIGINT, _trap_ctrl_c); -} + if ((_oldhandler = signal(SIGINT, _trap_ctrl_c)) == SIG_ERR) + return; -static void _remove_ctrl_c_handler() -{ - signal(SIGINT, SIG_IGN); - siginterrupt(SIGINT, 0); + sigprocmask(SIG_SETMASK, &_intsigset, NULL); + siginterrupt(SIGINT, 1); + + _handler_installed = 1; } static int _lock_file(const char *file, int flags) @@ -94,6 +116,7 @@ static int _lock_file(const char *file, int flags) int r = 1; struct lock_list *ll; + struct stat buf1, buf2; switch (flags & LCK_TYPE_MASK) { case LCK_READ: @@ -117,32 +140,45 @@ static int _lock_file(const char *file, int flags) return 0; } + ll->lf = -1; + log_very_verbose("Locking %s", ll->res); - if ((ll->lf = open(file, O_CREAT | O_APPEND | O_RDWR, 0777)) < 0) { - log_sys_error("open", file); - dbg_free(ll->res); - dbg_free(ll); - return 0; - } + do { + if (ll->lf > -1) + close(ll->lf); - if ((flags & LCK_NONBLOCK)) - operation |= LOCK_NB; - else - _install_ctrl_c_handler(); + if ((ll->lf = open(file, O_CREAT | O_APPEND | O_RDWR, 0777)) + < 0) { + log_sys_error("open", file); + goto err; + } - if (flock(ll->lf, operation)) { - log_sys_error("flock", ll->res); - dbg_free(ll->res); - dbg_free(ll); - r = 0; - } else - list_add(&_lock_list, &ll->list); + if ((flags & LCK_NONBLOCK)) + operation |= LOCK_NB; + else + _install_ctrl_c_handler(); + r = flock(ll->lf, operation); + if (!(flags & LCK_NONBLOCK)) + _remove_ctrl_c_handler(); - if (!(flags & LCK_NONBLOCK)) - _remove_ctrl_c_handler(); + if (r) { + log_sys_error("flock", ll->res); + goto err; + } - return r; + if (!stat(ll->res, &buf1) && !fstat(ll->lf, &buf2) && + !memcmp(&buf1.st_ino, &buf2.st_ino, sizeof(ino_t))) + break; + } while (!(flags & LCK_NONBLOCK)); + + list_add(&_lock_list, &ll->list); + return 1; + + err: + dbg_free(ll->res); + dbg_free(ll); + return 0; } int lock_resource(struct cmd_context *cmd, const char *resource, int flags) @@ -191,7 +227,6 @@ int lock_resource(struct cmd_context *cmd, const char *resource, int flags) return 1; } - int init_file_locking(struct locking_type *locking, struct config_file *cf) { locking->lock_resource = lock_resource; @@ -207,5 +242,15 @@ int init_file_locking(struct locking_type *locking, struct config_file *cf) list_init(&_lock_list); + if (sigfillset(&_intsigset) || sigfillset(&_fullsigset)) { + log_sys_error("sigfillset", "init_file_locking"); + return 0; + } + + if (sigdelset(&_intsigset, SIGINT)) { + log_sys_error("sigdelset", "init_file_locking"); + return 0; + } + return 1; } diff --git a/lib/locking/locking.c b/lib/locking/locking.c index a141536ba..cd7ab54d2 100644 --- a/lib/locking/locking.c +++ b/lib/locking/locking.c @@ -14,37 +14,45 @@ #include static struct locking_type _locking; +static sigset_t _oldset; -static int _lock_count = 0; /* Number of locks held */ -static int _signals_ignored = 0; +static int _lock_count = 0; /* Number of locks held */ +static int _signals_blocked = 0; -static void _ignore_signals(void) +static void _block_signals(void) { - int s; + sigset_t set; - if (_signals_ignored) + if (_signals_blocked) return; - for (s = 0; s < NSIG; s++) - signal(s, SIG_IGN); + if (sigfillset(&set)) { + log_sys_error("sigfillset", "_block_signals"); + return; + } - _signals_ignored = 1; + if (sigprocmask(SIG_SETMASK, &set, &_oldset)) { + log_sys_error("sigprocmask", "_block_signals"); + return; + } + + _signals_blocked = 1; return; } -static void _enable_signals(void) +static void _unblock_signals(void) { - int s; - - /* Don't enable signals while any locks are held */ - if (!_signals_ignored || _lock_count) + /* Don't unblock signals while any locks are held */ + if (!_signals_blocked || _lock_count) return; - for (s = 0; s < NSIG; s++) - signal(s, SIG_DFL); + if (sigprocmask(SIG_SETMASK, &_oldset, NULL)) { + log_sys_error("sigprocmask", "_block_signals"); + return; + } - _signals_ignored = 0; + _signals_blocked = 0; return; } @@ -70,7 +78,7 @@ void no_fin_locking(void) return; } -static void _init_no_locking(struct locking_type *locking, +static void _init_no_locking(struct locking_type *locking, struct config_file *cf) { locking->lock_resource = no_lock_resource; @@ -114,21 +122,20 @@ void fin_locking(void) } /* - * VG locking is by name - * LV locking is by VG_name/LV_uuid - * FIXME This should take a VG_uuid instead of VG_name + * VG locking is by VG name. + * FIXME This should become VG uuid. */ int _lock_vol(struct cmd_context *cmd, const char *resource, int flags) { - _ignore_signals(); + _block_signals(); if (!(_locking.lock_resource(cmd, resource, flags))) { - _enable_signals(); + _unblock_signals(); return 0; } _update_lock_count(flags); - _enable_signals(); + _unblock_signals(); return 1; } @@ -138,8 +145,8 @@ int lock_vol(struct cmd_context *cmd, const char *vol, int flags) char resource[258]; switch (flags & LCK_SCOPE_MASK) { - case LCK_VG: /* Lock volume group before changing on-disk metadata. */ - case LCK_LV: /* Suspends LV if it's active. */ + case LCK_VG: /* Lock VG to change on-disk metadata. */ + case LCK_LV: /* Suspends LV if it's active. */ strncpy(resource, (char *) vol, sizeof(resource)); break; default: @@ -153,11 +160,10 @@ int lock_vol(struct cmd_context *cmd, const char *vol, int flags) /* Perform immediate unlock unless LCK_HOLD set */ if (!(flags & LCK_HOLD) && ((flags & LCK_TYPE_MASK) != LCK_NONE)) { - if (!_lock_vol(cmd, resource, + if (!_lock_vol(cmd, resource, (flags & ~LCK_TYPE_MASK) | LCK_NONE)) return 0; } return 1; } - diff --git a/tools/toollib.c b/tools/toollib.c index ff60184a4..461450c06 100644 --- a/tools/toollib.c +++ b/tools/toollib.c @@ -134,7 +134,7 @@ int process_each_vg(struct cmd_context *cmd, int argc, char **argv, log_verbose("Using volume group(s) on command line"); for (; opt < argc; opt++) { vg_name = argv[opt]; - if (!lock_vol(cmd, vg_name, LCK_VG | lock_type)) { + if (!lock_vol(cmd, vg_name, lock_type)) { log_error("Can't lock %s: skipping", vg_name); continue; } @@ -150,7 +150,7 @@ int process_each_vg(struct cmd_context *cmd, int argc, char **argv, } list_iterate(vgh, vgs) { vg_name = list_item(vgh, struct name_list)->name; - if (!lock_vol(cmd, vg_name, LCK_VG | lock_type)) { + if (!lock_vol(cmd, vg_name, lock_type)) { log_error("Can't lock %s: skipping", vg_name); continue; } diff --git a/tools/vgcfgbackup.c b/tools/vgcfgbackup.c index 4d7c69ad6..93f65a179 100644 --- a/tools/vgcfgbackup.c +++ b/tools/vgcfgbackup.c @@ -54,5 +54,5 @@ static int vg_backup_single(struct cmd_context *cmd, const char *vg_name) int vgcfgbackup(struct cmd_context *cmd, int argc, char **argv) { - return process_each_vg(cmd, argc, argv, LCK_READ, &vg_backup_single); + return process_each_vg(cmd, argc, argv, LCK_VG_READ, &vg_backup_single); } diff --git a/tools/vgchange.c b/tools/vgchange.c index bdf2b1c41..f3dc08455 100644 --- a/tools/vgchange.c +++ b/tools/vgchange.c @@ -67,7 +67,7 @@ int vgchange(struct cmd_context *cmd, int argc, char **argv) return process_each_vg(cmd, argc, argv, (arg_count(cmd, available_ARG)) ? - LCK_READ : LCK_WRITE, &vgchange_single); + LCK_VG_READ : LCK_VG_WRITE, &vgchange_single); } static int vgchange_single(struct cmd_context *cmd, const char *vg_name) diff --git a/tools/vgck.c b/tools/vgck.c index d39ceeec3..12730b510 100644 --- a/tools/vgck.c +++ b/tools/vgck.c @@ -24,7 +24,7 @@ static int vgck_single(struct cmd_context *cmd, const char *vg_name); int vgck(struct cmd_context *cmd, int argc, char **argv) { - return process_each_vg(cmd, argc, argv, LCK_READ, &vgck_single); + return process_each_vg(cmd, argc, argv, LCK_VG_READ, &vgck_single); } static int vgck_single(struct cmd_context *cmd, const char *vg_name) diff --git a/tools/vgdisplay.c b/tools/vgdisplay.c index a191c312e..c3e494bac 100644 --- a/tools/vgdisplay.c +++ b/tools/vgdisplay.c @@ -45,7 +45,7 @@ int vgdisplay(struct cmd_context *cmd, int argc, char **argv) } **********/ - process_each_vg(cmd, argc, argv, LCK_READ, &vgdisplay_single); + process_each_vg(cmd, argc, argv, LCK_VG_READ, &vgdisplay_single); /******** FIXME Need to count number processed Add this to process_each_vg if arg_count(cmd,activevolumegroups_ARG) ? diff --git a/tools/vgexport.c b/tools/vgexport.c index 7605c6d8c..40502c89a 100644 --- a/tools/vgexport.c +++ b/tools/vgexport.c @@ -34,7 +34,7 @@ int vgexport(struct cmd_context *cmd, int argc, char **argv) return ECMD_FAILED; } - return process_each_vg(cmd, argc, argv, LCK_READ, &vgexport_single); + return process_each_vg(cmd, argc, argv, LCK_VG_READ, &vgexport_single); } static int vgexport_single(struct cmd_context *cmd, const char *vg_name) diff --git a/tools/vgimport.c b/tools/vgimport.c index 5aead1280..1d377eb2c 100644 --- a/tools/vgimport.c +++ b/tools/vgimport.c @@ -34,7 +34,7 @@ int vgimport(struct cmd_context *cmd, int argc, char **argv) return ECMD_FAILED; } - return process_each_vg(cmd, argc, argv, LCK_WRITE, &vgimport_single); + return process_each_vg(cmd, argc, argv, LCK_VG_WRITE, &vgimport_single); } static int vgimport_single(struct cmd_context *cmd, const char *vg_name) diff --git a/tools/vgremove.c b/tools/vgremove.c index 8ae46e3eb..8b9753b1c 100644 --- a/tools/vgremove.c +++ b/tools/vgremove.c @@ -31,7 +31,8 @@ int vgremove(struct cmd_context *cmd, int argc, char **argv) return ECMD_FAILED; } - ret = process_each_vg(cmd, argc, argv, LCK_WRITE | LCK_NONBLOCK, + ret = process_each_vg(cmd, argc, argv, + LCK_VG | LCK_WRITE | LCK_NONBLOCK, &vgremove_single); unlock_vg(cmd, ""); diff --git a/tools/vgscan.c b/tools/vgscan.c index be86d02b3..938553c41 100644 --- a/tools/vgscan.c +++ b/tools/vgscan.c @@ -37,7 +37,7 @@ int vgscan(struct cmd_context *cmd, int argc, char **argv) log_print("Reading all physical volumes. This may take a while..."); - return process_each_vg(cmd, argc, argv, LCK_READ, &vgscan_single); + return process_each_vg(cmd, argc, argv, LCK_VG_READ, &vgscan_single); } static int vgscan_single(struct cmd_context *cmd, const char *vg_name)