- Just fixed some style(9).
- Add BSD License and a proper copyright for the author.
Note: I did open another review because I don't want to command the review D7840.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 11776 Build 12120: arc lint + arc unit
Event Timeline
Comment Actions
There are a bunch of minor style(9) issues.
I was able to apply the diff and build cleanly. However, with an 11.1/amd64 guest and using mpg123 to play an MP3 file, I had an unimplemented instruction exit - I'll look more into this.
| usr.sbin/bhyve/audio.c | ||
|---|---|---|
| 64 | Not strict style(9) - variables declarations don't have initializers. | |
| 70 | style(9) "Values in return statements should be enclosed in parentheses" | |
| usr.sbin/bhyve/hda_codec.c | ||
| 144 | See style(9) on sorting struct elements. | |
| 870 | No need to cast a void * | |
| 921 | No error return to assert on here. | |
| 941 | Not in style(9), but the FreeBSD convention for function pointers is to make it explicit with a dereference e.g. use (*actx->do_setup)(actx->priv) | |
| usr.sbin/bhyve/hdac_reg.h | ||
| 270 | style(9), should be '#endif /* _HDAC_REG_H_ */ | |
| usr.sbin/bhyve/pci_hda.c | ||
| 297 | No need for return statements in void routines. | |
Comment Actions
- Address grehan@ request about some minor style(9) issues all over the place.
- Some structs were sorted by use and some by size and in alphabetical order.
- Remove the return from void functions.
| usr.sbin/bhyve/audio.c | ||
|---|---|---|
| 64 | There is nothing wrong to initialize the variable, we can do it explicit or let the compiler does it for us. | |
| usr.sbin/bhyve/hda_codec.c | ||
| 921 | Actually that assert should not be there according with pthread_set_name_np(3). | |
Comment Actions
I disabled capsicum by commenting out the cap_enter() call, and with the above struct revert, was able to cat a .WAV file and also use mpg123 to play a long mp3 file on a FreeBSD 11.1/amd64 guest, with a cheap USB audio adapter for output. Volume control seemed to work fine with mpg123. There was a very small glitch/skip when the audio was first played (or on resume after pause in mpg123).
The instruction-emlation patch for the OR instruction is at https://people.freebsd.org/~grehan/patches/vmm_instruction_emul.c.OR.diff
| usr.sbin/bhyve/pci_hda.c | ||
|---|---|---|
| 79 | I should have added that struct element ordering only applies for internal-use structs, and not ones that are used to represent in-memory device-emulation data structures (in this case, the HDA BDL struct). This one has to go back to the original ordering. You may want to have a look at any others that were re-ordered in case they were used for HDA device data structures. | |
Comment Actions
The OR emulation has been checked in with r325261
I've tested with Rhythmbox and browsers on Ubuntu 17.10, Edge and Skype (full audio call, audio in/out via HDA) on Win10.
Just need to add some Capsicum-ization and this will be good to go.
Comment Actions
The OR emulation has been checked in with r325261
I've tested with Rhythmbox and browsers on Ubuntu 17.10, Edge and Skype (full audio call, audio in/out via HDA) on Win10.
Just need to add some Capsicum-ization and this will be good to go.
Comment Actions
Any update on this? I've tried this diff on a recent current but it seems like it doesn't work anymore?
Comment Actions
I will come back to this patch soon. I'm waiting some capsicum patches do land. I didn't have enough time to check if those were committed or not.
Comment Actions
Just a hands up, this patch needs the following patches:
Committed already: https://reviews.freebsd.org/D14557
Pending: https://reviews.freebsd.org/D14407
Comment Actions
Hi @araujo, are there plans to move forward with this now that https://reviews.freebsd.org/rS340373 (libcasper: introduce cap_fileargs service) was commited?
Comment Actions
Yes, definitely! I'm working on it, I got some issues with cap_fileargs and I have asked Mariuz support to help me debug the issue. But yeah, I'm on it right now!
Comment Actions
I have implemented cap_fileargs(3) and now the driver is in capabilities mode and works relatively fine.
Still there are some adjustments that I need to make and I hope till next week I will update this review with the new code.
Comment Actions
If anybody else is interested on this patch, ping me and I can share my latest changes. However I'm not interested on it anymore.
Comment Actions
For anybody interested in this: This patch correctly applies to 12.0-Release-Stable.
I would like to see more development on this as it provides a native bandaid to the lack of WideVine DRM/EME support.
Comment Actions
Is anybody still working on this?
As far as I can see it's ready to get committed. So let's get this into bhyve after all these years...
Comment Actions
Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.
Comment Actions
Please read the whole review, Peter Grehan points out several times that it has capsicum issues, and that it has only been shown to work with Capsicum disabled.