[alsa-devel] [PATCH 2/2] ALSA: ctl: refactoring for read operation

[Takashi Iwai]

At Fri, 10 Apr 2015 19:32:58 +0900,
Takashi Sakamoto wrote:
> 
> On Apr 10 2015 16:48, Takashi Iwai wrote:
> > At Fri, 10 Apr 2015 08:43:01 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> snd_ctl_read() has nested loop and complicated condition for return
> >> value. This is not better for reading.
> >>
> >> This commit applies refactoring with two loops.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> >> ---
> >>  sound/core/control.c | 77 ++++++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 47 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/sound/core/control.c b/sound/core/control.c
> >> index de19d56..da6497d 100644
> >> --- a/sound/core/control.c
> >> +++ b/sound/core/control.c
> >> @@ -1520,58 +1520,75 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer,
> >>  			    size_t count, loff_t * offset)
> >>  {
> >>  	struct snd_ctl_file *ctl;
> >> -	int err = 0;
> >> -	ssize_t result = 0;
> >> +	struct snd_ctl_event ev;
> >> +	struct snd_kctl_event *kev;
> >> +	wait_queue_t wait;
> >> +	ssize_t result;
> >>  
> >>  	ctl = file->private_data;
> >>  	if (snd_BUG_ON(!ctl || !ctl->card))
> >>  		return -ENXIO;
> >>  	if (!ctl->subscribed)
> >>  		return -EBADFD;
> >> +
> >> +	/* The size of given buffer should be larger than at least one event. */
> >>  	if (count < sizeof(struct snd_ctl_event))
> >>  		return -EINVAL;
> >> +
> >> +	/* Block till any events were queued. */
> >>  	spin_lock_irq(&ctl->read_lock);
> >> -	while (count >= sizeof(struct snd_ctl_event)) {
> >> -		struct snd_ctl_event ev;
> >> -		struct snd_kctl_event *kev;
> >> -		while (list_empty(&ctl->events)) {
> >> -			wait_queue_t wait;
> >> -			if ((file->f_flags & O_NONBLOCK) != 0 || result > 0) {
> >> -				err = -EAGAIN;
> >> -				goto __end_lock;
> >> -			}
> >> -			init_waitqueue_entry(&wait, current);
> >> -			add_wait_queue(&ctl->change_sleep, &wait);
> >> -			set_current_state(TASK_INTERRUPTIBLE);
> >> -			spin_unlock_irq(&ctl->read_lock);
> >> -			schedule();
> >> -			remove_wait_queue(&ctl->change_sleep, &wait);
> >> -			if (ctl->card->shutdown)
> >> -				return -ENODEV;
> >> -			if (signal_pending(current))
> >> -				return -ERESTARTSYS;
> >> -			spin_lock_irq(&ctl->read_lock);
> >> +	while (list_empty(&ctl->events)) {
> >> +		if (file->f_flags & O_NONBLOCK) {
> >> +			result = -EAGAIN;
> >> +			goto unlock;
> >>  		}
> >> +
> >> +		/* The card was disconnected. The queued events are dropped. */
> >> +		if (ctl->card->shutdown) {
> >> +			result = -ENODEV;
> >> +			goto unlock;
> >> +		}
> >> +
> >> +
> >> +		init_waitqueue_entry(&wait, current);
> >> +		add_wait_queue(&ctl->change_sleep, &wait);
> >> +		set_current_state(TASK_INTERRUPTIBLE);
> >> +		spin_unlock_irq(&ctl->read_lock);
> >> +
> >> +		schedule();
> >> +
> >> +		remove_wait_queue(&ctl->change_sleep, &wait);
> >> +
> >> +		if (signal_pending(current))
> >> +			return -ERESTARTSYS;
> >> +
> >> +		spin_lock_irq(&ctl->read_lock);
> >> +	}
> >> +
> >> +	/* Copy events till the buffer filled, or no events are remained. */
> >> +	result = 0;
> >> +	while (count >= sizeof(struct snd_ctl_event)) {
> >> +		if (list_empty(&ctl->events))
> >> +			break;
> >>  		kev = snd_kctl_event(ctl->events.next);
> >> +		list_del(&kev->list);
> >> +
> >>  		ev.type = SNDRV_CTL_EVENT_ELEM;
> >>  		ev.data.elem.mask = kev->mask;
> >>  		ev.data.elem.id = kev->id;
> >> -		list_del(&kev->list);
> >> -		spin_unlock_irq(&ctl->read_lock);
> >>  		kfree(kev);
> >>  		if (copy_to_user(buffer, &ev, sizeof(struct snd_ctl_event))) {
> >> -			err = -EFAULT;
> >> -			goto __end;
> >> +			result = -EFAULT;
> >> +			break;
> > 
> > This still changes the behavior.  The read returns the size that has
> > been read at first even when an error happens if some data was already
> > read.  The error code is returned at the next read.  This is a design
> > problem of read syscall.
> 
> I'm unaware of it... These code should be:
> 
> if (copy_to_user(buffer, &ev, sizeof(struct snd_ctl_event))) {
>     if (result == 0)
>         result = -EFAULT;
>     break;

Well, it's not much better than the original code, IMO.

> And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same
> bug. I'll post another patch for this bug.

This is an implementation detail and I believe it rather depends on
each driver.  (But I should reread POSIX definition again before
concluding it.)

That said, it isn't wrong to return -EFAULT immediately, per se.
The problem here is that you change the existing behavior.  Especially
a core code like this should be treated carefully even for such a
small behavior change.


Takashi