[PATCHv2 alsa-lib 0/3] Close-on-exec flag for device nodes

[PATCHv2 alsa-lib 0/3] Close-on-exec flag for device nodes

[Rémi Denis-Courmont]

        Hello,

This is basically the same patch series as yesterday, but addressing Lennart's 
issue. Comments welcome.

 configure.in             |    2 ++
 include/local.h          |   17 +++++++++++------
 src/control/control_hw.c |   11 -----------
 src/hwdep/hwdep_hw.c     |   11 -----------
 src/pcm/pcm_hw.c         |   12 ------------
 src/rawmidi/rawmidi_hw.c |   11 -----------
 src/seq/seq_hw.c         |   11 -----------
 src/timer/timer_hw.c     |   11 -----------
 8 files changed, 13 insertions(+), 73 deletions(-)


-- 
Rémi Denis-Courmont
http://www.remlab.net/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 1/3] Open device nodes with close-on-exec flag

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 include/local.h |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/local.h b/include/local.h
index b5a1c45..afc4acd 100644
--- a/include/local.h
+++ b/include/local.h
@@ -230,22 +230,27 @@ extern snd_lib_error_handler_t snd_err_msg;
 # define link_warning(symbol, msg)
 #endif
 
-/* open with resmgr */
-#ifdef SUPPORT_RESMGR
 static inline int snd_open_device(const char *filename, int fmode)
 {
+#ifdef O_CLOEXEC
+	int fd = open(filename, fmode|O_CLOEXEC);
+#else
 	int fd = open(filename, fmode);
-	if (fd >= 0)
+#endif
+	if (fd >= 0) {
+		fcntl(fd, F_SETFD, FD_CLOEXEC);
 		return fd;
+	}
+
+/* open with resmgr */
+#ifdef SUPPORT_RESMGR
 	if (errno == EAGAIN || errno == EBUSY)
 		return fd;
 	if (! access(filename, F_OK))
 		return rsm_open_device(filename, fmode);
+#endif
 	return -1;
 }
-#else
-#define snd_open_device(filename, fmode) open(filename, fmode);
-#endif
 
 /* make local functions really local */
 #define snd_dlobj_cache_lookup \
-- 
1.6.5.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/3] Remove old commented-out FD_CLOEXEC code

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 src/control/control_hw.c |   11 -----------
 src/hwdep/hwdep_hw.c     |   11 -----------
 src/pcm/pcm_hw.c         |   12 ------------
 src/rawmidi/rawmidi_hw.c |   11 -----------
 src/seq/seq_hw.c         |   11 -----------
 src/timer/timer_hw.c     |   11 -----------
 6 files changed, 0 insertions(+), 67 deletions(-)

diff --git a/src/control/control_hw.c b/src/control/control_hw.c
index e9a6be2..cf258b4 100644
--- a/src/control/control_hw.c
+++ b/src/control/control_hw.c
@@ -392,17 +392,6 @@ int snd_ctl_hw_open(snd_ctl_t **handle, const char *name, int card, int mode)
 		if (fd < 0)
 			return -errno;
 	}
-#if 0
-	/*
-	 * this is bogus, an application have to care about open filedescriptors
-	 */
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		SYSERR("fcntl FD_CLOEXEC failed");
-		err = -errno;
-		close(fd);
-		return err;
-	}
-#endif
 	if (ioctl(fd, SNDRV_CTL_IOCTL_PVERSION, &ver) < 0) {
 		err = -errno;
 		close(fd);
diff --git a/src/hwdep/hwdep_hw.c b/src/hwdep/hwdep_hw.c
index 238a507..e4fcdc3 100644
--- a/src/hwdep/hwdep_hw.c
+++ b/src/hwdep/hwdep_hw.c
@@ -122,17 +122,6 @@ int snd_hwdep_hw_open(snd_hwdep_t **handle, const char *name, int card, int devi
 		if (fd < 0)
 			return -errno;
 	}
-#if 0
-	/*
-	 * this is bogus, an application have to care about open filedescriptors
-	 */
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		SYSERR("fcntl FD_CLOEXEC failed");
-		ret = -errno;
-		close(fd);
-		return ret;
-	}
-#endif
 	if (ioctl(fd, SNDRV_HWDEP_IOCTL_PVERSION, &ver) < 0) {
 		ret = -errno;
 		close(fd);
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 8abb204..2095b01 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -1144,18 +1144,6 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
 	if (fmode & O_ASYNC)
 		mode |= SND_PCM_ASYNC;
 
-#if 0
-	/*
-	 * this is bogus, an application have to care about open filedescriptors
-	 */
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		ret = -errno;
-		SYSMSG("fcntl FD_CLOEXEC failed");
-		close(fd);
-		return ret;
-	}
-#endif
-
 	if (ioctl(fd, SNDRV_PCM_IOCTL_PVERSION, &ver) < 0) {
 		ret = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_PVERSION failed");
diff --git a/src/rawmidi/rawmidi_hw.c b/src/rawmidi/rawmidi_hw.c
index 87829fd..e08dca7 100644
--- a/src/rawmidi/rawmidi_hw.c
+++ b/src/rawmidi/rawmidi_hw.c
@@ -234,17 +234,6 @@ int snd_rawmidi_hw_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
 			return -errno;
 		}
 	}
-#if 0
-	/*
-	 * this is bogus, an application have to care about open filedescriptors
-	 */
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		SYSERR("fcntl FD_CLOEXEC failed");
-		ret = -errno;
-		close(fd);
-		return ret;
-	}
-#endif
 	if (ioctl(fd, SNDRV_RAWMIDI_IOCTL_PVERSION, &ver) < 0) {
 		ret = -errno;
 		SYSERR("SNDRV_RAWMIDI_IOCTL_PVERSION failed");
diff --git a/src/seq/seq_hw.c b/src/seq/seq_hw.c
index 0e94593..5bb26f3 100644
--- a/src/seq/seq_hw.c
+++ b/src/seq/seq_hw.c
@@ -457,17 +457,6 @@ int snd_seq_hw_open(snd_seq_t **handle, const char *name, int streams, int mode)
 		SYSERR("open %s failed", filename);
 		return -errno;
 	}
-#if 0
-	/*
-         * this is bogus, an application have to care about open filedescriptors
-	 */                          
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		SYSERR("fcntl FD_CLOEXEC failed");
-		ret = -errno;
-		close(fd);
-		return ret;
-	}
-#endif
 	if (ioctl(fd, SNDRV_SEQ_IOCTL_PVERSION, &ver) < 0) {
 		SYSERR("SNDRV_SEQ_IOCTL_PVERSION failed");
 		ret = -errno;
diff --git a/src/timer/timer_hw.c b/src/timer/timer_hw.c
index 3453858..25f3bab 100644
--- a/src/timer/timer_hw.c
+++ b/src/timer/timer_hw.c
@@ -236,17 +236,6 @@ int snd_timer_hw_open(snd_timer_t **handle, const char *name, int dev_class, int
 	fd = snd_open_device(SNDRV_FILE_TIMER, tmode);
 	if (fd < 0)
 		return -errno;
-#if 0
-	/*
-	 * this is bogus, an application have to care about open filedescriptors
-	 */
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0) {
-		SYSERR("fcntl FD_CLOEXEC failed");
-		ret = -errno;
-		close(fd);
-		return ret;
-	}
-#endif
 	if (ioctl(fd, SNDRV_TIMER_IOCTL_PVERSION, &ver) < 0) {
 		ret = -errno;
 		close(fd);
-- 
1.6.5.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 3/3] Define _GNU_SOURCE so that <fcntl.h> gives O_CLOEXEC

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 configure.in |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/configure.in b/configure.in
index a455de1..cc8950f 100644
--- a/configure.in
+++ b/configure.in
@@ -38,6 +38,8 @@ then
   AC_MSG_RESULT($CC)
 fi
 	    
+CFLAGS="$CFLAGS -D_GNU_SOURCE"
+
 
 AC_PROG_CC
 AC_PROG_CPP
-- 
1.6.5.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] Open device nodes with close-on-exec flag

[Takashi Iwai]

At Thu,  5 Nov 2009 21:17:40 +0200,
Rémi Denis-Courmont wrote:
> 
> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
>  include/local.h |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/include/local.h b/include/local.h
> index b5a1c45..afc4acd 100644
> --- a/include/local.h
> +++ b/include/local.h
> @@ -230,22 +230,27 @@ extern snd_lib_error_handler_t snd_err_msg;
>  # define link_warning(symbol, msg)
>  #endif
>  
> -/* open with resmgr */
> -#ifdef SUPPORT_RESMGR
>  static inline int snd_open_device(const char *filename, int fmode)
>  {
> +#ifdef O_CLOEXEC
> +	int fd = open(filename, fmode|O_CLOEXEC);
> +#else
>  	int fd = open(filename, fmode);
> -	if (fd >= 0)
> +#endif
> +	if (fd >= 0) {
> +		fcntl(fd, F_SETFD, FD_CLOEXEC);
>  		return fd;
> +	}
> +
> +/* open with resmgr */
> +#ifdef SUPPORT_RESMGR
>  	if (errno == EAGAIN || errno == EBUSY)
>  		return fd;
>  	if (! access(filename, F_OK))
>  		return rsm_open_device(filename, fmode);
> +#endif
>  	return -1;
>  }
> -#else
> -#define snd_open_device(filename, fmode) open(filename, fmode);
> -#endif

CLOEXEC stuff should be applied to the case with resmgr as well.
Care to rewrite the patch?


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] Open device nodes with close-on-exec flag

[Rémi Denis-Courmont]

	Hello,

Le vendredi 6 novembre 2009 12:06:41 Takashi Iwai, vous avez écrit :
> CLOEXEC stuff should be applied to the case with resmgr as well.
> Care to rewrite the patch?

Sure. There you go... (patches 2 and 3 unchanged)

-- 
Rémi Denis-Courmont
http://www.remlab.net/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 1/3] Open device nodes with close-on-exec flag

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 include/local.h |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/local.h b/include/local.h
index b5a1c45..fa3f0b7 100644
--- a/include/local.h
+++ b/include/local.h
@@ -230,22 +230,28 @@ extern snd_lib_error_handler_t snd_err_msg;
 # define link_warning(symbol, msg)
 #endif
 
-/* open with resmgr */
-#ifdef SUPPORT_RESMGR
 static inline int snd_open_device(const char *filename, int fmode)
 {
-	int fd = open(filename, fmode);
+	int fd;
+
+#ifdef O_CLOEXEC
+	fmode |= O_CLOEXEC;
+#endif
+	fd = open(filename, fmode);
+
+/* open with resmgr */
+#ifdef SUPPORT_RESMGR
+	if (fd < 0) {
+		if (errno == EAGAIN || errno == EBUSY)
+			return fd;
+		if (! access(filename, F_OK))
+			fd = rsm_open_device(filename, fmode);
+	}
+#endif
 	if (fd >= 0)
-		return fd;
-	if (errno == EAGAIN || errno == EBUSY)
-		return fd;
-	if (! access(filename, F_OK))
-		return rsm_open_device(filename, fmode);
-	return -1;
+		fcntl(fd, F_SETFD, FD_CLOEXEC);
+	return fd;
 }
-#else
-#define snd_open_device(filename, fmode) open(filename, fmode);
-#endif
 
 /* make local functions really local */
 #define snd_dlobj_cache_lookup \
-- 
1.6.5.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] Open device nodes with close-on-exec flag

[Takashi Iwai]

At Sat, 7 Nov 2009 21:28:35 +0200,
=?utf-8?q?R=C3=A9mi?= Denis-Courmont wrote:
> 
> 	Hello,
> 
> Le vendredi 6 novembre 2009 12:06:41 Takashi Iwai, vous avez écrit :
> > CLOEXEC stuff should be applied to the case with resmgr as well.
> > Care to rewrite the patch?
> 
> Sure. There you go... (patches 2 and 3 unchanged)

Thanks, all applied now.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] Define _GNU_SOURCE so that <fcntl.h> gives O_CLOEXEC

[Lennart Poettering]

On Thu, 05.11.09 21:17, Rémi Denis-Courmont (remi@remlab.net) wrote:

> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
>  configure.in |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index a455de1..cc8950f 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -38,6 +38,8 @@ then
>    AC_MSG_RESULT($CC)
>  fi
>  	    
> +CFLAGS="$CFLAGS -D_GNU_SOURCE"

There's acually an autoconf macro for this. AC_GNU_SOURCE. And even
better than that is usually AC_SYSTEM_EXTENSIONS.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH 3/3] Define _GNU_SOURCE so that <fcntl.h> gives O_CLOEXEC

[Rémi Denis-Courmont]

Le dimanche 8 novembre 2009 16:49:37 Lennart Poettering, vous avez écrit :
> On Thu, 05.11.09 21:17, Rémi Denis-Courmont (remi@remlab.net) wrote:
> > Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> > ---
> >  configure.in |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/configure.in b/configure.in
> > index a455de1..cc8950f 100644
> > --- a/configure.in
> > +++ b/configure.in
> > @@ -38,6 +38,8 @@ then
> >    AC_MSG_RESULT($CC)
> >  fi
> >
> > +CFLAGS="$CFLAGS -D_GNU_SOURCE"
> 
> There's acually an autoconf macro for this. AC_GNU_SOURCE. And even
> better than that is usually AC_SYSTEM_EXTENSIONS.

AC_SYSTEM_EXTENSIONS is not much extra help from AC_GNU_SOURCE in a Linux-only 
library. alsa-lib does not systematically include <config.h> first, so neither 
of them will work anyway.

I did not dare to make such a big change. We need _GNU_SOURCE before *any* 
system header is included. Hence I added it to C(PP)FLAGS.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [PATCH 3/3] Define _GNU_SOURCE so that <fcntl.h> gives O_CLOEXEC

[Lennart Poettering]

On Sun, 08.11.09 17:23, Rémi Denis-Courmont (remi@remlab.net) wrote:

> 
> Le dimanche 8 novembre 2009 16:49:37 Lennart Poettering, vous avez écrit :
> > On Thu, 05.11.09 21:17, Rémi Denis-Courmont (remi@remlab.net) wrote:
> > > Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> > > ---
> > >  configure.in |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/configure.in b/configure.in
> > > index a455de1..cc8950f 100644
> > > --- a/configure.in
> > > +++ b/configure.in
> > > @@ -38,6 +38,8 @@ then
> > >    AC_MSG_RESULT($CC)
> > >  fi
> > >
> > > +CFLAGS="$CFLAGS -D_GNU_SOURCE"
> > 
> > There's acually an autoconf macro for this. AC_GNU_SOURCE. And even
> > better than that is usually AC_SYSTEM_EXTENSIONS.
> 
> AC_SYSTEM_EXTENSIONS is not much extra help from AC_GNU_SOURCE in a Linux-only 
> library. 

In the past there was some work to port it to some BSD iirc, using the
OSS backend that is currently in alsa-plugins. Not that I cared a tiny
bit about BSD but I generally think that if there are two otherwise
equal options one should pick the portable one. But then again, this
certainly doesn't matter much.

> alsa-lib does not systematically include <config.h> first, so neither 
> of them will work anyway.

A nice fix for that could be simply adding -include
$(top_builddir)/config.h to AM_CPPFLAGS. A couple of packages do that
now, e.g. udev.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH 1/3] Open device nodes with close-on-exec flag

[Lennart Poettering]

On Wed, 04.11.09 20:38, Rémi Denis-Courmont (remi@remlab.net) wrote:

>  static inline int snd_open_device(const char *filename, int fmode)
>  {
> -	int fd = open(filename, fmode);
> +	int fd;
> +
> +#ifdef O_CLOEXEC
> +	fd = open(filename, fmode|O_CLOEXEC);
>  	if (fd >= 0)
>  		return fd;
> +	if (errno == EINVAL)
> +#endif
> +	{
> +		fd = open(filename, fmode);
> +		if (fd >= 0) {
> +			fcntl(fd, F_SETFD, FD_CLOEXEC);
> +			return fd;
> +		}

That's actually not how things works. O_CLOEXEC is silently ignored by
old kernels. (The calls with SOCK_CLOEXEC fail with EINVAL while the
calls with O_CLOEXEC silently succeed) You need to set
FD_CLOEXEC unconditionally to cover all cases:

fd = open(fn, fmode
#ifdef O_CLOEXEC
          |O_CLOEXEC
#endif
);
fcntl(fd, F_SETFD, FD_CLOEXEC);


Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

[PATCH 1/3] Open device nodes with close-on-exec flag

[Rémi Denis-Courmont]

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 include/local.h |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/local.h b/include/local.h
index b5a1c45..5b54def 100644
--- a/include/local.h
+++ b/include/local.h
@@ -230,22 +230,33 @@ extern snd_lib_error_handler_t snd_err_msg;
 # define link_warning(symbol, msg)
 #endif
 
-/* open with resmgr */
-#ifdef SUPPORT_RESMGR
 static inline int snd_open_device(const char *filename, int fmode)
 {
-	int fd = open(filename, fmode);
+	int fd;
+
+#ifdef O_CLOEXEC
+	fd = open(filename, fmode|O_CLOEXEC);
 	if (fd >= 0)
 		return fd;
+	if (errno == EINVAL)
+#endif
+	{
+		fd = open(filename, fmode);
+		if (fd >= 0) {
+			fcntl(fd, F_SETFD, FD_CLOEXEC);
+			return fd;
+		}
+	}
+
+/* open with resmgr */
+#ifdef SUPPORT_RESMGR
 	if (errno == EAGAIN || errno == EBUSY)
 		return fd;
 	if (! access(filename, F_OK))
 		return rsm_open_device(filename, fmode);
+#endif
 	return -1;
 }
-#else
-#define snd_open_device(filename, fmode) open(filename, fmode);
-#endif
 
 /* make local functions really local */
 #define snd_dlobj_cache_lookup \
-- 
1.6.5.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel