Summary:	ASTERISK-07645: [patch][post 1.4] Configurable lock types for ast_lock_path - allows voicemail storage on SMB/CIFS mounts
Reporter:	Nic Bellamy (nic_bellamy)	Labels:
Date Opened:	2006-08-31 16:29:45	Date Closed:	2007-08-28 11:10:19
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 2006-10-03_svn_44249_voicemail_lockmode_v3.patch
Description:	This patch adds a "lockmode = {lockfile|flock}" parameter to the [options] section of asterisk.conf. This allows you to set which type of locking ast_lock_path() uses. It defaults to the current behaviour (ie. create a random file, hardlink it to dir/.lock), which isn't quite NFS-safe, but is close. The flock mode instead creates (if it doesn't already exist) a dir/lock file, and then flock()s it - this allows it to lock paths on filesystems that don't support filenames starting with "." and/or hardlinks, eg. SMB/CIFS mounted filesystems. ****** ADDITIONAL INFORMATION ****** The only thing I don't like about this patch is that when in flock mode it leaves lock files around in every directory it locks. However, I can't see a way around this without creating race conditions. I've filed this under app_voicemail as it's presently the only in-tree user of ast_lock_path/ast_unlock_path. I have a version of this patch for 1.2.x also if anyone wants to try it there. Disclaimer on file under Vadacom Limited, NZ.
Comments:	By: Anthony LaMantia (alamantia) 2006-09-26 14:52:45 I have made a testing branch in my subversion directory for testing this patch.. it's location is http://svn.digium.com/view/asterisk/team/anthonyl/7852-patchtest/ if anyone cares to test it and provide feedback it would be great. we still need to preform a code reivew By: Nic Bellamy (nic_bellamy) 2006-09-26 16:34:48 I'm obviously biased, being the original submitter, but we're using the 1.2 version of this patch (which is basically identical) on about 20 production systems now, about 5 of which are flash-based and have voicemail storage on Windows servers via smbfs/cifs. So far, the only issue was an accidential use of flock locking on an NFS mounted voicemail store; NFS lockd wasn't running, which caused locking to fail - and voicemail to be discarded (which isn't the locking subsystems fault, but isn't good either). As for coding style, I've stuck to the guidelines, even the bits I think are insane^W^W^W don't agree with ;-) By: Anthony LaMantia (alamantia) 2006-09-27 16:49:58 it looks good, i would like to see a nice way of removing the old lock files after  a call to ast_unlock_path_flock() .. By: Nic Bellamy (nic_bellamy) 2006-09-27 17:32:01 I'll see if I can figure out a way to remove them when unlocking without creating a race condition. By: Anthony LaMantia (alamantia) 2006-09-29 15:05:40 cool thanks we'll be waiting. By: Nic Bellamy (nic_bellamy) 2006-09-30 16:45:08 After much thinking, reading and Googling, I was nearly in despair at being able to find a race-free way of removing the lockfiles - but I stumbled across a technique that Exim and UW-IMAP use for exactly this situation - after getting the lock, lstat the lockfile and fstat our open descriptor, and compare the device and inode to ensure it hasn't changed underneath us. New patch uploaded, with the following changes: * Fix accidental usage of fprintf() * Fix an error case where the lock file would not be closed, leaking a file descriptor * Remove lockfile when unlocking path (and add associated checks in the locking code to mitigate create/unlink races) By: Serge Vecher (serge-v) 2006-10-02 08:58:27 nic: sorry to neat pick here -- single line if ... else constructs do not need curly brackets .... By: Nic Bellamy (nic_bellamy) 2006-10-02 22:54:42 serge-v: the only place my patch uses that construct is in main/asterisk.c, and in that case, that's the prevaling style of the rest of the code around it. That's what you were talking about? Uploading v3 patch in a moment anyway - it removes the {} for the final else in main/asterisk.c, and uses ast_calloc rather than malloc + memset. By: jmls (jmls) 2006-11-03 03:40:38.000-0600 can we get this reviewed ? Thanks By: Serge Vecher (serge-v) 2007-03-06 14:16:32.000-0600 does this issue enter the ast_storage territory? By: Digium Subversion (svnbot) 2007-08-28 11:10:18 Repository: asterisk Revision: 81233 ------------------------------------------------------------------------ r81233 | russell | 2007-08-28 11:10:17 -0500 (Tue, 28 Aug 2007) | 10 lines (closes issue ASTERISK-7645) Reported by: nic_bellamy Patches:      2006-10-03_svn_44249_voicemail_lockmode_v3.patch uploaded by nic_bellamy (license 213) Add support for configurable file locking methods.  The default is "lockfile", which is the old behavior.  There is an additional option, "flock", which is intended for use in situations where the lockfile method will not work, such as with SMB/CIFS mounts. ------------------------------------------------------------------------