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

[Takashi Iwai]

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 at 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 at suse.de>


thanks,

Takashi