| Message ID | 1428622981-9747-3-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. Takashi > } > - spin_lock_irq(&ctl->read_lock); > + > buffer += sizeof(struct snd_ctl_event); > count -= sizeof(struct snd_ctl_event); > result += sizeof(struct snd_ctl_event); > } > - __end_lock: > +unlock: > spin_unlock_irq(&ctl->read_lock); > - __end: > - return result > 0 ? result : err; > + return result; > } > > static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) > -- > 2.1.0 >
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@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; And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same bug. I'll post another patch for this bug. Thanks Takashi Sakamoto > Takashi > >> } >> - spin_lock_irq(&ctl->read_lock); >> + >> buffer += sizeof(struct snd_ctl_event); >> count -= sizeof(struct snd_ctl_event); >> result += sizeof(struct snd_ctl_event); >> } >> - __end_lock: >> +unlock: >> spin_unlock_irq(&ctl->read_lock); >> - __end: >> - return result > 0 ? result : err; >> + return result; >> } >> >> static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) >> -- >> 2.1.0
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@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
On Apr 10 2015 19:43, Takashi Iwai wrote: >> 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. OK. I dropped this patch. >> 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. Thanks Takashi Sakamoto
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; } - spin_lock_irq(&ctl->read_lock); + buffer += sizeof(struct snd_ctl_event); count -= sizeof(struct snd_ctl_event); result += sizeof(struct snd_ctl_event); } - __end_lock: +unlock: spin_unlock_irq(&ctl->read_lock); - __end: - return result > 0 ? result : err; + return result; } static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
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@sakamocchi.jp> --- sound/core/control.c | 77 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 30 deletions(-)