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@xxxxxxxx>
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@xxxxxxx>
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
