[PATCH] alsactl: Try to create state file directory

[PATCH] alsactl: Try to create state file directory

[Dan Nicholson]

Try to create the directory for the state file when saving so we don't
depend on it being created ahead of time. This only checks for failures
on existing directories and doesn't try to create the leading
directories or workaround any other errors. This should catch the common
case where /var/lib exists, but /var/lib/alsa doesn't.

Signed-off-by: Dan Nicholson <nicholson@endlessm.com>
---
 alsactl/state.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/alsactl/state.c b/alsactl/state.c
index 3908ec4..8ca3d6e 100644
--- a/alsactl/state.c
+++ b/alsactl/state.c
@@ -27,6 +27,9 @@
 #include <stdio.h>
 #include <assert.h>
 #include <errno.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <alsa/asoundlib.h>
 #include "alsactl.h"
 
@@ -1544,6 +1547,7 @@ int save_state(const char *file, const char *cardname)
 	snd_output_t *out;
 	int stdio;
 	char *nfile = NULL;
+	char *filedir = NULL;
 	int lock_fd = -EINVAL;
 
 	err = snd_config_top(&config);
@@ -1553,6 +1557,9 @@ int save_state(const char *file, const char *cardname)
 	}
 	stdio = !strcmp(file, "-");
 	if (!stdio) {
+		char *tmp;
+		mode_t mode = S_IRWXU | S_IRWXG | S_IRWXO;
+
 		nfile = malloc(strlen(file) + 5);
 		if (nfile == NULL) {
 			error("No enough memory...");
@@ -1561,6 +1568,21 @@ int save_state(const char *file, const char *cardname)
 		}
 		strcpy(nfile, file);
 		strcat(nfile, ".new");
+		filedir = strdup(file);
+		if (filedir == NULL) {
+			error("Not enough memory...");
+			err = -ENOMEM;
+			goto out;
+		}
+		tmp = strrchr(filedir, '/');
+		if (tmp && tmp != filedir) {
+			*tmp = '\0';
+			if (mkdir(filedir, mode) != 0 && errno != EEXIST) {
+				error("Could not create directory %s: %s",
+				      filedir, strerror(errno));
+				goto out;
+			}
+		}
 		lock_fd = state_lock(file, 10);
 		if (lock_fd < 0) {
 			err = lock_fd;
@@ -1640,6 +1662,7 @@ out:
 	if (!stdio && lock_fd >= 0)
 		state_unlock(lock_fd, file);
 	free(nfile);
+	free(filedir);
 	snd_config_delete(config);
 	snd_config_update_free_global();
 	return err;
-- 
1.8.1.2

[PATCH] alsactl: Try to create state file directory

[Dan Nicholson]

Try to create the directory for the state file when saving so we don't
depend on it being created ahead of time. This only checks for failures
on existing directories and doesn't try to create the leading
directories or workaround any other errors. This should catch the common
case where /var/lib exists, but /var/lib/alsa doesn't.

Signed-off-by: Dan Nicholson <nicholson@endlessm.com>
---
v2: I forgot to set err if mkdir failed.

 alsactl/state.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/alsactl/state.c b/alsactl/state.c
index 3908ec4..faa1579 100644
--- a/alsactl/state.c
+++ b/alsactl/state.c
@@ -27,6 +27,9 @@
 #include <stdio.h>
 #include <assert.h>
 #include <errno.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <alsa/asoundlib.h>
 #include "alsactl.h"
 
@@ -1544,6 +1547,7 @@ int save_state(const char *file, const char *cardname)
 	snd_output_t *out;
 	int stdio;
 	char *nfile = NULL;
+	char *filedir = NULL;
 	int lock_fd = -EINVAL;
 
 	err = snd_config_top(&config);
@@ -1553,6 +1557,9 @@ int save_state(const char *file, const char *cardname)
 	}
 	stdio = !strcmp(file, "-");
 	if (!stdio) {
+		char *tmp;
+		mode_t mode = S_IRWXU | S_IRWXG | S_IRWXO;
+
 		nfile = malloc(strlen(file) + 5);
 		if (nfile == NULL) {
 			error("No enough memory...");
@@ -1561,6 +1568,22 @@ int save_state(const char *file, const char *cardname)
 		}
 		strcpy(nfile, file);
 		strcat(nfile, ".new");
+		filedir = strdup(file);
+		if (filedir == NULL) {
+			error("Not enough memory...");
+			err = -ENOMEM;
+			goto out;
+		}
+		tmp = strrchr(filedir, '/');
+		if (tmp && tmp != filedir) {
+			*tmp = '\0';
+			if (mkdir(filedir, mode) != 0 && errno != EEXIST) {
+				error("Could not create directory %s: %s",
+				      filedir, strerror(errno));
+				err = -errno;
+				goto out;
+			}
+		}
 		lock_fd = state_lock(file, 10);
 		if (lock_fd < 0) {
 			err = lock_fd;
@@ -1640,6 +1663,7 @@ out:
 	if (!stdio && lock_fd >= 0)
 		state_unlock(lock_fd, file);
 	free(nfile);
+	free(filedir);
 	snd_config_delete(config);
 	snd_config_update_free_global();
 	return err;
-- 
1.8.1.2

Re: [PATCH] alsactl: Try to create state file directory

[Takashi Iwai]

At Fri,  5 Jun 2015 15:00:47 -0700,
Dan Nicholson wrote:
> 
> Try to create the directory for the state file when saving so we don't
> depend on it being created ahead of time. This only checks for failures
> on existing directories and doesn't try to create the leading
> directories or workaround any other errors. This should catch the common
> case where /var/lib exists, but /var/lib/alsa doesn't.

I don't think it's the role of alsactl.  It saves a file on the
certain directory.  If it doesn't exist, it's a failure of the
installed package.


thanks,

Takashi

> 
> Signed-off-by: Dan Nicholson <nicholson@endlessm.com>
> ---
>  alsactl/state.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/alsactl/state.c b/alsactl/state.c
> index 3908ec4..8ca3d6e 100644
> --- a/alsactl/state.c
> +++ b/alsactl/state.c
> @@ -27,6 +27,9 @@
>  #include <stdio.h>
>  #include <assert.h>
>  #include <errno.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
>  #include <alsa/asoundlib.h>
>  #include "alsactl.h"
>  
> @@ -1544,6 +1547,7 @@ int save_state(const char *file, const char *cardname)
>  	snd_output_t *out;
>  	int stdio;
>  	char *nfile = NULL;
> +	char *filedir = NULL;
>  	int lock_fd = -EINVAL;
>  
>  	err = snd_config_top(&config);
> @@ -1553,6 +1557,9 @@ int save_state(const char *file, const char *cardname)
>  	}
>  	stdio = !strcmp(file, "-");
>  	if (!stdio) {
> +		char *tmp;
> +		mode_t mode = S_IRWXU | S_IRWXG | S_IRWXO;
> +
>  		nfile = malloc(strlen(file) + 5);
>  		if (nfile == NULL) {
>  			error("No enough memory...");
> @@ -1561,6 +1568,21 @@ int save_state(const char *file, const char *cardname)
>  		}
>  		strcpy(nfile, file);
>  		strcat(nfile, ".new");
> +		filedir = strdup(file);
> +		if (filedir == NULL) {
> +			error("Not enough memory...");
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +		tmp = strrchr(filedir, '/');
> +		if (tmp && tmp != filedir) {
> +			*tmp = '\0';
> +			if (mkdir(filedir, mode) != 0 && errno != EEXIST) {
> +				error("Could not create directory %s: %s",
> +				      filedir, strerror(errno));
> +				goto out;
> +			}
> +		}
>  		lock_fd = state_lock(file, 10);
>  		if (lock_fd < 0) {
>  			err = lock_fd;
> @@ -1640,6 +1662,7 @@ out:
>  	if (!stdio && lock_fd >= 0)
>  		state_unlock(lock_fd, file);
>  	free(nfile);
> +	free(filedir);
>  	snd_config_delete(config);
>  	snd_config_update_free_global();
>  	return err;
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] alsactl: Try to create state file directory

[Dan Nicholson]

On Jun 8, 2015 4:38 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
>
> At Fri,  5 Jun 2015 15:00:47 -0700,
> Dan Nicholson wrote:
> >
> > Try to create the directory for the state file when saving so we don't
> > depend on it being created ahead of time. This only checks for failures
> > on existing directories and doesn't try to create the leading
> > directories or workaround any other errors. This should catch the common
> > case where /var/lib exists, but /var/lib/alsa doesn't.
>
> I don't think it's the role of alsactl.  It saves a file on the
> certain directory.  If it doesn't exist, it's a failure of the
> installed package.

Sure, that's understandable, but there's a couple reasons I think this is
helpful addition.

First, if no path is supplied, store will save to /var/lib/alsa. So, it's
not as of the user has supplied a path it didn't setup correctly. It would
be nice if alsactl worked out of the box without additional integration by
packagers.

Second, my real motivation for fixing this is to support stateless type of
systems that come with a clean /var. At Endless we're using ostree. The OS
is composed by Debian packages, and indeed alsa-utils is setup to create
/var/lib/alsa on install. However, to use the same OS snapshot for all
users, the contents of /var are stripped since they represent local system
state. We can certainly add a method for creating the directory at runtime,
but we believe it's more robust to have the program manage its own state as
much as possible.

Thanks,
Dan

Re: [PATCH] alsactl: Try to create state file directory

[Takashi Iwai]

At Mon, 8 Jun 2015 05:52:18 -0700,
Dan Nicholson wrote:
> 
> On Jun 8, 2015 4:38 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
> >
> > At Fri,  5 Jun 2015 15:00:47 -0700,
> > Dan Nicholson wrote:
> > >
> > > Try to create the directory for the state file when saving so we don't
> > > depend on it being created ahead of time. This only checks for failures
> > > on existing directories and doesn't try to create the leading
> > > directories or workaround any other errors. This should catch the common
> > > case where /var/lib exists, but /var/lib/alsa doesn't.
> >
> > I don't think it's the role of alsactl.  It saves a file on the
> > certain directory.  If it doesn't exist, it's a failure of the
> > installed package.
> 
> Sure, that's understandable, but there's a couple reasons I think this is
> helpful addition.
> 
> First, if no path is supplied, store will save to /var/lib/alsa. So, it's
> not as of the user has supplied a path it didn't setup correctly. It would
> be nice if alsactl worked out of the box without additional integration by
> packagers.

For that, a safer way would be to create /var/lib/alsa in the
installation.

> Second, my real motivation for fixing this is to support stateless type of
> systems that come with a clean /var. At Endless we're using ostree. The OS
> is composed by Debian packages, and indeed alsa-utils is setup to create
> /var/lib/alsa on install. However, to use the same OS snapshot for all
> users, the contents of /var are stripped since they represent local system
> state. We can certainly add a method for creating the directory at runtime,
> but we believe it's more robust to have the program manage its own state as
> much as possible.

Why not specifying the proper directory via alsactl -f option for
user?


Takashi

Re: [PATCH] alsactl: Try to create state file directory

[Takashi Iwai]

At Mon, 08 Jun 2015 15:03:21 +0200,
Takashi Iwai wrote:
> 
> At Mon, 8 Jun 2015 05:52:18 -0700,
> Dan Nicholson wrote:
> > 
> > On Jun 8, 2015 4:38 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
> > >
> > > At Fri,  5 Jun 2015 15:00:47 -0700,
> > > Dan Nicholson wrote:
> > > >
> > > > Try to create the directory for the state file when saving so we don't
> > > > depend on it being created ahead of time. This only checks for failures
> > > > on existing directories and doesn't try to create the leading
> > > > directories or workaround any other errors. This should catch the common
> > > > case where /var/lib exists, but /var/lib/alsa doesn't.
> > >
> > > I don't think it's the role of alsactl.  It saves a file on the
> > > certain directory.  If it doesn't exist, it's a failure of the
> > > installed package.
> > 
> > Sure, that's understandable, but there's a couple reasons I think this is
> > helpful addition.
> > 
> > First, if no path is supplied, store will save to /var/lib/alsa. So, it's
> > not as of the user has supplied a path it didn't setup correctly. It would
> > be nice if alsactl worked out of the box without additional integration by
> > packagers.
> 
> For that, a safer way would be to create /var/lib/alsa in the
> installation.
> 
> > Second, my real motivation for fixing this is to support stateless type of
> > systems that come with a clean /var. At Endless we're using ostree. The OS
> > is composed by Debian packages, and indeed alsa-utils is setup to create
> > /var/lib/alsa on install. However, to use the same OS snapshot for all
> > users, the contents of /var are stripped since they represent local system
> > state. We can certainly add a method for creating the directory at runtime,
> > but we believe it's more robust to have the program manage its own state as
> > much as possible.
> 
> Why not specifying the proper directory via alsactl -f option for
> user?

Also, your patch seems to care only the top directory.  For example,
if alsactl is configured to use /var/lib/alsa/more/deep/dir, it won't
work.


thanks,

Takashi

Re: [PATCH] alsactl: Try to create state file directory

[Dan Nicholson]

On Mon, 2015-06-08 at 15:22 +0200, Takashi Iwai wrote:
> At Mon, 08 Jun 2015 15:03:21 +0200,
> Takashi Iwai wrote:
> > 
> > At Mon, 8 Jun 2015 05:52:18 -0700,
> > Dan Nicholson wrote:
> > > 
> > > On Jun 8, 2015 4:38 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
> > > >
> > > > At Fri,  5 Jun 2015 15:00:47 -0700,
> > > > Dan Nicholson wrote:
> > > > >
> > > > > Try to create the directory for the state file when saving so we don't
> > > > > depend on it being created ahead of time. This only checks for failures
> > > > > on existing directories and doesn't try to create the leading
> > > > > directories or workaround any other errors. This should catch the common
> > > > > case where /var/lib exists, but /var/lib/alsa doesn't.
> > > >
> > > > I don't think it's the role of alsactl.  It saves a file on the
> > > > certain directory.  If it doesn't exist, it's a failure of the
> > > > installed package.
> > > 
> > > Sure, that's understandable, but there's a couple reasons I think this is
> > > helpful addition.
> > > 
> > > First, if no path is supplied, store will save to /var/lib/alsa. So, it's
> > > not as of the user has supplied a path it didn't setup correctly. It would
> > > be nice if alsactl worked out of the box without additional integration by
> > > packagers.
> > 
> > For that, a safer way would be to create /var/lib/alsa in the
> > installation.'s 

Sure, as I said, we can do that if necessary. It's just another custom
thing to maintain, and I think it would be nice if alsactl could manage
this on its own.

> > > Second, my real motivation for fixing this is to support stateless type of
> > > systems that come with a clean /var. At Endless we're using ostree. The OS
> > > is composed by Debian packages, and indeed alsa-utils is setup to create
> > > /var/lib/alsa on install. However, to use the same OS snapshot for all
> > > users, the contents of /var are stripped since they represent local system
> > > state. We can certainly add a method for creating the directory at runtime,
> > > but we believe it's more robust to have the program manage its own state as
> > > much as possible.
> > 
> > Why not specifying the proper directory via alsactl -f option for
> > user?
> 
> Also, your patch seems to care only the top directory.  For example,
> if alsactl is configured to use /var/lib/alsa/more/deep/dir, it won't
> work.

Here's another version that creates all leading directories.

>From e1f75388682ad662dd5bdbb057d12b00e0aa80a8 Mon Sep 17 00:00:00 2001
From: Dan Nicholson <nicholson@endlessm.com>
Date: Fri, 5 Jun 2015 14:39:15 -0700
Subject: [PATCH] alsactl: Try to create state file directory

Try to create the directory for the state file when saving so we don't
depend on it being created ahead of time. This only checks for failures
on existing directories. This should catch the common case where
/var/lib exists, but /var/lib/alsa doesn't.

Signed-off-by: Dan Nicholson <nicholson@endlessm.com>
---
v3: Create all leading directories

 alsactl/state.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/alsactl/state.c b/alsactl/state.c
index 3908ec4..10b3567 100644
--- a/alsactl/state.c
+++ b/alsactl/state.c
@@ -27,6 +27,9 @@
 #include <stdio.h>
 #include <assert.h>
 #include <errno.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <alsa/asoundlib.h>
 #include "alsactl.h"
 
@@ -1536,6 +1539,41 @@ static int set_controls(int card, snd_config_t
*top, int doit)
 	return err;
 }
 
+static int create_directories(const char *file)
+{
+	int err = 0;
+	char *filedir, *cur;
+
+	filedir = strdup(file);
+	if (filedir == NULL) {
+		error("Not enough memory...");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	cur = filedir;
+	/* skip leading /s */
+	while (*cur && *cur == '/')
+		cur++;
+	while ((cur = strchr(cur, '/')) != NULL) {
+		mode_t mode = S_IRWXU | S_IRWXG | S_IRWXO;
+
+		*cur = '\0';
+		if (mkdir(filedir, mode) != 0 && errno != EEXIST) {
+			error("Could not create directory %s: %s",
+			      filedir, strerror(errno));
+			err = -errno;
+			goto out;
+		}
+		/* restore path element */
+		*cur++ = '/';
+	}
+
+out:
+	free(filedir);
+	return err;
+}
+
 int save_state(const char *file, const char *cardname)
 {
 	int err;
@@ -1561,6 +1599,9 @@ int save_state(const char *file, const char
*cardname)
 		}
 		strcpy(nfile, file);
 		strcat(nfile, ".new");
+		err = create_directories(file);
+		if (err < 0)
+			goto out;
 		lock_fd = state_lock(file, 10);
 		if (lock_fd < 0) {
 			err = lock_fd;
-- 
1.8.1.2

Re: [PATCH] alsactl: Try to create state file directory

[Takashi Iwai]

At Mon, 08 Jun 2015 09:17:49 -0700,
Dan Nicholson wrote:
> 
> On Mon, 2015-06-08 at 15:22 +0200, Takashi Iwai wrote:
> > At Mon, 08 Jun 2015 15:03:21 +0200,
> > Takashi Iwai wrote:
> > > 
> > > At Mon, 8 Jun 2015 05:52:18 -0700,
> > > Dan Nicholson wrote:
> > > > 
> > > > On Jun 8, 2015 4:38 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
> > > > >
> > > > > At Fri,  5 Jun 2015 15:00:47 -0700,
> > > > > Dan Nicholson wrote:
> > > > > >
> > > > > > Try to create the directory for the state file when saving so we don't
> > > > > > depend on it being created ahead of time. This only checks for failures
> > > > > > on existing directories and doesn't try to create the leading
> > > > > > directories or workaround any other errors. This should catch the common
> > > > > > case where /var/lib exists, but /var/lib/alsa doesn't.
> > > > >
> > > > > I don't think it's the role of alsactl.  It saves a file on the
> > > > > certain directory.  If it doesn't exist, it's a failure of the
> > > > > installed package.
> > > > 
> > > > Sure, that's understandable, but there's a couple reasons I think this is
> > > > helpful addition.
> > > > 
> > > > First, if no path is supplied, store will save to /var/lib/alsa. So, it's
> > > > not as of the user has supplied a path it didn't setup correctly. It would
> > > > be nice if alsactl worked out of the box without additional integration by
> > > > packagers.
> > > 
> > > For that, a safer way would be to create /var/lib/alsa in the
> > > installation.'s 
> 
> Sure, as I said, we can do that if necessary. It's just another custom
> thing to maintain, and I think it would be nice if alsactl could manage
> this on its own.

Well, it can show a different face depending on the perspective.
Imagine if you pass a wrong path (e.g. /vat/lib/alsa/asound.state).
Then it creates the whole wrong directories.  So, it might be nice,
but it might be dangerous.  And, as it's a tool mostly used by root,
I'd like to vote for safety.


thanks,

Takashi