On 01/13/2011 03:21 PM, Eric Blake wrote:
On 01/13/2011 08:45 AM, Cole Robinson wrote:
In QEMU, the card itself is a PCI device, but it requires a codec (either -device hda-output or -device hda-duplex) to actually output sound. We set up an hda-duplex codec by default: I think it's important that a simple <sound model='ich6'/> sets up a useful codec, to have consistent behavior with all other sound cards.
This is basically Dan's proposal of
<sound model='ich6'> <codec type='output' slot='0'/> <codec type='duplex' slot='3'/> </sound>
without the codec bits implemented.
Reasonable for a first round of patches.
The important thing is to keep a consistent API here, we don't want some <sound> models to require tweaking codecs but not others. Steps I see to accomplishing this when someone gets around to it:
- every <sound> device has a <codec type='default'/> (unless codecs are manually specified) - <codec type='none'/> is required to specify 'no codecs' - new audio settings like mic=on|off could then be exposed in <sound> or <codec> in a consistent manner for all sound models
Agree that those are good steps forward, and that they do not hold up this patch.
+++ b/src/qemu/qemu_command.c @@ -1727,11 +1727,13 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound) goto error; }
- /* Hack for 2 wierdly unusal devices name in QEMU */ + /* Hack for wierdly unusal devices name in QEMU */
As long as you're touching the comment: s/wierdly/weirdly/
Will fix.
@@ -3751,6 +3753,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error;
virCommandAddArg(cmd, str); + + if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { + virCommandAddArgList(cmd, + "-device", "hda-duplex", NULL); + } +
Should this come with a qemu_capabilities.[ch] check that device hda-duplex is available? Or are we relying on the fact that qemu will exit with a sane error message if an unsupported device is requested?
The patch looks fine to me once you fix the spelling nit, but I'd rather get an answer to whether qemu_capabilities should be changed before giving ack.
Ideally we would just rely on qemu to report a useful error in this case, but instead it gives us: $ x86_64-softmmu/qemu-system-x86_64 -device foobar qemu-system-x86_64: -device foobar: Parameter 'driver' expects a driver name Try with argument '?' for a list. I consider that a qemu bug though. I've filed a report against RHEL6: https://bugzilla.redhat.com/show_bug.cgi?id=669524 I'd rather error out than just ignore the unsupported device, so I don't think a capabilities check buys us much besides working around a suboptimal error message (which will hopefully be fixed soon anyways) Thanks, Cole