Re: [PATCH v2] alsactl: Store lockfile in /tmp

[Julian Scheel <julian@jusst.de>]

Am 07.05.2014 09:19, schrieb Takashi Iwai:
> At Tue,  6 May 2014 21:32:19 +0200,
> Julian Scheel wrote:
>>
>> It can not be generally assumed that the directories in which asound.state
>> resides are writable. Instead using /tmp as location for lock files seems more
>> reliable.
>
> The subject and changelog don't match with the actual change.
> Now it's /var/log instead of /tmp, right?

Sorry for that. Must have been too late.

> Besides that, it'd be better to allow a full path name for a lock file
> instead of a directory name.  If you give a different file name via -f
> option, you have a high chance to conflict with the existing file in
> /var/lock.

So, you'd prefer a --lock-file/-L option which can be used to set an 
explicit lock file?

> Furthermore, for solving *your* problem (restoring from read-only
> rootfs), an easier option would be allowing to restore the system
> default without locking.

While this is true I think making the locking mechanism more robust is a 
good thing anyway.

Julian

>
> Takashi
>
>>
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>> ---
>>   alsactl/alsactl.c |  7 +++++++
>>   alsactl/alsactl.h |  1 +
>>   alsactl/lock.c    | 13 ++++++++++---
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/alsactl/alsactl.c b/alsactl/alsactl.c
>> index 6bc013f..415dfb8 100644
>> --- a/alsactl/alsactl.c
>> +++ b/alsactl/alsactl.c
>> @@ -38,6 +38,9 @@
>>   #ifndef SYS_PIDFILE
>>   #define SYS_PIDFILE "/var/run/alsactl.pid"
>>   #endif
>> +#ifndef SYS_LOCKPATH
>> +#define SYS_LOCKPATH "/var/lock"
>> +#endif
>>
>>   int debugflag = 0;
>>   int force_restore = 1;
>> @@ -46,6 +49,7 @@ int do_lock = 0;
>>   int use_syslog = 0;
>>   char *command;
>>   char *statefile = NULL;
>> +char *lockpath = SYS_LOCKPATH;
>>
>>   #define TITLE	0x0100
>>   #define HEADER	0x0200
>> @@ -71,6 +75,7 @@ static struct arg args[] = {
>>   { HEADER, NULL, "Available state options:" },
>>   { FILEARG | 'f', "file", "configuration file (default " SYS_ASOUNDRC ")" },
>>   { 'l', "lock", "use file locking to serialize concurrent access" },
>> +{ FILEARG | 'D', "lock-dir", "directory to use for lock files (default " SYS_LOCKPATH ")" },
>>   { 'F', "force", "try to restore the matching controls as much as possible" },
>>   { 0, NULL, "  (default mode)" },
>>   { 'g', "ignore", "ignore 'No soundcards found' error" },
>> @@ -232,6 +237,8 @@ int main(int argc, char *argv[])
>>   		case 'l':
>>   			do_lock = 1;
>>   			break;
>> +		case 'D':
>> +			lockpath = optarg;
>>   		case 'F':
>>   			force_restore = 1;
>>   			break;
>> diff --git a/alsactl/alsactl.h b/alsactl/alsactl.h
>> index 9109a70..6c6bee5 100644
>> --- a/alsactl/alsactl.h
>> +++ b/alsactl/alsactl.h
>> @@ -5,6 +5,7 @@ extern int do_lock;
>>   extern int use_syslog;
>>   extern char *command;
>>   extern char *statefile;
>> +extern char *lockpath;
>>
>>   void info_(const char *fcn, long line, const char *fmt, ...);
>>   void error_(const char *fcn, long line, const char *fmt, ...);
>> diff --git a/alsactl/lock.c b/alsactl/lock.c
>> index 587a109..c69e285 100644
>> --- a/alsactl/lock.c
>> +++ b/alsactl/lock.c
>> @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout)
>>   	struct flock lck;
>>   	struct stat st;
>>   	char lcktxt[12];
>> +	char *filename;
>>   	char *nfile;
>>
>>   	if (!do_lock)
>>   		return 0;
>> -	nfile = malloc(strlen(file) + 6);
>> +
>> +	/* only use the actual filename, not the path */
>> +	filename = strrchr(file, '/');
>> +	if (!filename)
>> +		filename = file;
>> +
>> +	nfile = malloc(strlen(lockpath) + strlen(filename) + 7);
>>   	if (nfile == NULL) {
>>   		error("No enough memory...");
>>   		return -ENOMEM;
>>   	}
>> -	strcpy(nfile, file);
>> -	strcat(nfile, ".lock");
>> +
>> +	sprintf(nfile, "%s/%s.lock", lockpath, filename);
>>   	lck.l_type = lock ? F_WRLCK : F_UNLCK;
>>   	lck.l_whence = SEEK_SET;
>>   	lck.l_start = 0;
>> --
>> 1.9.2
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>