[v2] alsactl: Store lockfile in /tmp
diff mbox

Message ID 1399404739-1007-1-git-send-email-julian@jusst.de
State Accepted
Headers show

Commit Message

Julian Scheel May 6, 2014, 7:32 p.m. UTC
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.

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(-)

Comments

Takashi Iwai May 7, 2014, 7:19 a.m. UTC | #1
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?

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.

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


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
>
Julian Scheel May 7, 2014, 8:49 a.m. UTC | #2
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
>>
Takashi Iwai May 7, 2014, 9:20 a.m. UTC | #3
At Wed, 07 May 2014 10:49:59 +0200,
Julian Scheel wrote:
> 
> 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.

Seems so...

> > 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?

Yes.

> > 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.

Of course, it's good to improve the lock mechanism.
OTOH, adding an option like --no-lock would be good, too.
This is a different solution, and the idea doesn't conflict.


Takashi

> 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
> >>
>
Jaroslav Kysela May 7, 2014, 9:23 a.m. UTC | #4
Date 7.5.2014 10:49, Julian Scheel wrote:
> 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.

I also didn't note that. I applied your v2 patch to the git repo - I
forced update now.

>> 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?

I changed '-D' to '-O' option (file) and used '-L' option to select the
"no-lock" behaviour for the global configuration file. Note that the
locking is default only for the global file, other files are not lock
protected.

http://git.alsa-project.org/?p=alsa-utils.git;a=commitdiff;h=158a67f6f5058bec0ac27086a1c6206bfd2ff414

>> 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.

Yup.

					Jaroslav
Julian Scheel May 7, 2014, 1:17 p.m. UTC | #5
Am 07.05.2014 11:23, schrieb Jaroslav Kysela:
> Date 7.5.2014 10:49, Julian Scheel wrote:
>> 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.
>
> I also didn't note that. I applied your v2 patch to the git repo - I
> forced update now.

Thank you and sorry again :)

>>> 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?
>
> I changed '-D' to '-O' option (file) and used '-L' option to select the
> "no-lock" behaviour for the global configuration file. Note that the
> locking is default only for the global file, other files are not lock
> protected.

Alright, that looks good.

-Julian

> http://git.alsa-project.org/?p=alsa-utils.git;a=commitdiff;h=158a67f6f5058bec0ac27086a1c6206bfd2ff414
>
>>> 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.
>
> Yup.
>
> 					Jaroslav
>

Patch
diff mbox

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;