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

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/

> @@ -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.