rc: virtual_oss: Some bug fixes #33
emaste
markj
jrm
0mp
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "christos/rc"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
@ -740,0 +748,4 @@-b 24 \-s 8ms \-i 8 \-f /dev/dsp$(sysctl -n hw.snd.default_unit 2>/dev/null) \The sysctl command here does not take effect. Is there a way to make this work somehow?
I tend to agree with @Omp that
/etc/defaults/rc.confmay not be the best place to callsysctl. That said, I think this is working for me. Right after a reboot, I seevirtual_ossis running as4660 - S<s 0:00.68 /usr/sbin/virtual_oss -B -D /var/run/virtual_oss/dsp.pid -S -C 2 -c 2 -r 48000 -b 24 -s 8ms -i 8 -f /dev/dsp3 -d dsp -t vdsp.ctl. Could there be a race condition where the sysctl may run before or after the devices are created?@jrm wrote in #33 (comment):
Probably. I think it's better to do what Mateusz said indeed.
87913bd1783e6783dcfdrc: virtual_oss: Define variables in rc.confto rc: virtual_oss: Some bug fixes@ -740,0 +740,4 @@virtual_oss_enable="NO" # Run virtual_oss at startup.virtual_oss_default_control_device="vdsp.ctl" # Default configuration's control device.virtual_oss_configs="dsp" # List of configurations. Default is "dsp".virtual_oss_dsp="\Probably this should just live in the rc script and any overrides should be handled differently. There is always the standard
*_flagsbut the exact semantics in the case of virtual_oss will require some thinking.@ -740,0 +750,4 @@-i 8 \-f /dev/dsp$(sysctl -n hw.snd.default_unit 2>/dev/null) \-d dsp \-t ${virtual_oss_default_control_device}" # Default configuration.This will be broken as well if someone sets a custom virtual_oss_default_control_device
5b98f648bf454abddea7454abddea762dedd593a@ -740,0 +739,4 @@virtual_oss_enable="NO" # Run virtual_oss at startup.virtual_oss_configs="dsp" # List of configurations.# Default is "dsp", defined inI think that lines 742 and 743 are not strictly necessary. I'd recommend removing them, but I leave the decision to you.
These are documented in rc.conf.5, so I guess we could remove them indeed. Updating the review.
62dedd593a451304c3ed@ -93,0 +83,4 @@pid="$(cat "${pidfile}")"startmsg -n "Stopping virtual_oss config: ${config}: "kill "${pid}"pwait "${pid}"I'm testing this, and I'm seeing
service virtual_oss stophang indefinitely. It would be helpful to add a timeout to thepwaitcall.Maybe something like this (untested):
Let's talk about this more this coming week.
Why is virtual_oss refusing to terminate?
It looks like this happens when virtual_oss gets killed during I/O. But worse than this, if you stop the
servicecommand, and then look at ps, virtual_oss is still there:Manually issuing a
kill -KILLafterwards does not do anything.This seems unrelated to this PR though. The same problems occurs even if you manually try to kill a virtual_oss instance during I/O. I think we could go ahead with this PR, and debug this problem separately.
Can we go through with this? Especially the rc.conf commit fixes a bug that's already attracted a PR [1]. As I said in the inline comments, the hang is unrelated to these patches, and has to do with virtual_oss(8), not the rc scripts.
[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=295560
You can consider "rc: virtual_oss: Define some variables in rc.conf" approved by me.
Regarding remaining commits: it's actually better to just call
run_rc_command stopto handle pidfiles, signal sending, and waiting. But should be taken care of in another PR.Despite my original comments, I agree with this. I think it would also be good to get this into the tree so more people can test it.
451304c3edb2677d002e