| Message ID | 1399404739-1007-1-git-send-email-julian@jusst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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 >>
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 > >> >
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
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 >
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;
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(-)