Re: [alsa-devel] [PATCH 1/2] ALSA: pcm: implement the anonymous dup (inode file descriptor)

On Wed, 30 Jan 2019 09:47:32 +0100, Jaroslav Kysela wrote:

This patch implements new SNDRV_PCM_IOCTL_ANONYMOUS_DUP ioctl which returns the new duplicated anonymous inode file descriptor (anon_inode:snd-pcm) which can be passed to the restricted clients.

This patch is meant to be the alternative for the dma-buf interface. Both implementation have some pros and cons:

anon_inode:dmabuf

• a bit standard export API for the DMA buffers
• fencing for the concurrent access [1]
• driver/kernel interface for the DMA buffer [1]
• multiple attach/detach scheme [1]

[1] the real usage for the sound PCM is unknown at the moment for this feature

anon_inode:snd-pcm

• simple (no problem with ref-counting, non-standard mmap implementation etc.)
• allow to use more sound interfaces for the file descriptor like status ioctls
• more fine grained security policies (another anon_inode name unshared with other drivers)

Signed-off-by: Jaroslav Kysela perex@perex.cz

This version is indeed shorter, that's good!

Just minor nitpicking:

+static int snd_pcm_anonymous_dup(struct file *file,

• 		 struct snd_pcm_substream *substream,
  

• 		 int __user *arg)
  

+{

• int fd;
• int err;
• int perm;
• int flags;
• struct file *nfile;
• struct snd_pcm *pcm = substream->pcm;

You prefer rather a normal Christmas tree? ;)

• if (get_user(perm, (int __user *)arg))

The cast looks superfluous.

• return -EFAULT;
  

• if (perm < 0)

• return -EPERM;
  

• flags = file->f_flags & (O_ACCMODE | O_NONBLOCK);
• flags |= O_APPEND | O_CLOEXEC;
• fd = get_unused_fd_flags(flags);
• if (fd < 0)

• return fd;
  

• nfile = anon_inode_getfile("snd-pcm", file->f_op, NULL, flags);
• if (IS_ERR(nfile)) {

• put_unused_fd(fd);
  

• return PTR_ERR(nfile);
  

• }
• /* anon_inode_getfile() filters the O_APPEND flag out */
• nfile->f_flags |= O_APPEND;
• fd_install(fd, nfile);
• if (!try_module_get(pcm->card->module)) {

• err = -EFAULT;
  

EFAULT doesn't appear to be the best fitting for this error path.

• goto __error1;
  

• }
• err = snd_card_file_add(substream->pcm->card, nfile);

substream->pcm can be replaced with pcm.

• if (err < 0)

• goto __error2;
  

• err = snd_pcm_open_file(nfile, substream->pcm,

Ditto.

• 		substream->stream, substream->number);
  

• if (err >= 0) {

• put_user(fd, (int __user *)arg);
  

The cast looks superfluous.

• return 0;
  

• }
• snd_card_file_remove(substream->pcm->card, nfile);

Ditto.

Other than these small things, LGTM; Reviewed-by: Takashi Iwai tiwai@suse.de

thanks,

Takashi