Paths

Table of Contentst

sound: Do not fail from vchan_destroy() if children list is empty
Needs ReviewPublic
Actions

Authored by christos on Mon, Dec 23, 10:30 PM.

Details

Reviewers

markj
emaste
dev_submerge.ch

Summary

vchan_destroy() should be able to call chn_kill() regardless of whether
the channel has been added to the children list yet or not. Also remove
the CHN_F_BUSY check. There is no reason we shouldn't execute the code
below if the parent is not busy.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 61400
Build 58284: arc lint + arc unit

Event Timeline

christos created this revision.Mon, Dec 23, 10:30 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, Dec 23, 10:30 PM

christos requested review of this revision.Mon, Dec 23, 10:30 PM

Harbormaster completed remote builds in B61314: Diff 148358.Mon, Dec 23, 10:30 PM

christos mentioned this in D48183: sound: Call vchan_destroy() on vchan_create() failure.Mon, Dec 23, 10:30 PM

christos added a child revision: D48183: sound: Call vchan_destroy() on vchan_create() failure.

christos added inline comments.Mon, Dec 23, 10:33 PM

sys/dev/sound/pcm/vchan.c
845	Neither this nor the left diff guarantee us that the vchan we are removing from the list has actually been added. Perhaps it's better to use `CHN_REMOVE_SAFE` instead?

dev_submerge.ch added inline comments.Thu, Dec 26, 3:57 PM

sys/dev/sound/pcm/vchan.c
845	Do I see that correctly, `SLIST_REMOVE()` will just happily dereference the terminal NULL pointer of the list, when the element to be removed is not present? Yes, I think `CHN_REMOVE_SAFE` is a good choice here.

dev_submerge.ch mentioned this in D48207: sound: Safely remove channel from list in one pass..Thu, Dec 26, 11:53 PM

christos marked an inline comment as done.Fri, Dec 27, 6:18 PM

christos added inline comments.

sys/dev/sound/pcm/vchan.c
845	Yes, there is no checking whatsoever.

christos edited the summary of this revision. (Show Details)Fri, Dec 27, 6:23 PM

Use CHN_REMOVE_SAFE().

Harbormaster completed remote builds in B61356: Diff 148436.Fri, Dec 27, 6:25 PM

dev_submerge.ch added inline comments.Sat, Dec 28, 2:55 PM

sys/dev/sound/pcm/vchan.c
845	This check shouldn't be necessary anymore. But how about an assertion that the channel isn't in use, per `CHN_F_BUSY` flag?

Remove CHN_EMPTY check

sys/dev/sound/pcm/vchan.c
845	I agree with removing the check. However, I am actually not sure whether the correct behavior here is to not go through with the deletion if the channel is busy or to just force it.

Harbormaster completed remote builds in B61397: Diff 148536.Sun, Dec 29, 7:56 PM

christos edited the summary of this revision. (Show Details)Sun, Dec 29, 8:54 PM

Rebase on top of D48249

Harbormaster completed remote builds in B61400: Diff 148540.Sun, Dec 29, 8:55 PM

christos added a parent revision: D48249: sound: Make CHN_REMOVE_SAFE() the default.Sun, Dec 29, 8:59 PM

dev_submerge.ch added inline comments.Sun, Dec 29, 9:50 PM

sys/dev/sound/pcm/vchan.c
845	Sure, at this point we should go through with the deletion. An assertion would just help to catch programming errors where the vchan is not properly closed and shut down first, in debug builds.

Bump.

Revision Contents
Changeset List

Path

Size

sys/

dev/

sound/

pcm/

vchan.c

7 lines

Diff 148540

View Options

sound: Do not fail from vchan_destroy() if children list is emptyNeeds ReviewPublicActions