[libvirt] [PATCH v2] qemu: sound: Support intel 'ich6' model

[Cole Robinson]

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