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

[Cole Robinson]

On 01/14/2011 11:20 AM, Eric Blake wrote:
> On 01/13/2011 02:45 PM, Cole Robinson wrote:
>>>> @@ -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?
>>>
> 
>> 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
> 
> But even if that gets patched, your proposed error message is still
> rather weak:
> 
> qemu-system-x86_64: -device foobar: unknown device 'foobar'
> 
> Whereas my recent patch to make qemu device parsing much easier could
> let us do:
> 
> diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
> index f967255..b70c583 100644
> --- i/src/qemu/qemu_capabilities.c
> +++ w/src/qemu/qemu_capabilities.c
> @@ -1046,6 +1046,7 @@ qemuCapsExtractDeviceStr(const char *qemu,
>       * isolation, but are silently ignored in combination with
>       * '-device ?'.  */
>      cmd = virCommandNewArgList(qemu,
> +                               "-device", "?",
>                                 "-device", "pci-assign,?",
>                                 NULL);
>      virCommandAddEnvPassCommon(cmd);
> @@ -1068,6 +1069,11 @@ cleanup:
>  int
>  qemuCapsParseDeviceStr(const char *str, unsigned long long *flags)
>  {
> +    /* Which devices exist. */
> +    if (strstr(str, "name \"hda-duplex\""))
> +        *flags |= QEMUD_CMD_FLAG_HDA_DUPLEX;
> +
> +    /* Features of given devices. */
>      if (strstr(str, "pci-assign.configfd"))
>          *flags |= QEMUD_CMD_FLAG_PCI_CONFIGFD;
> 
> diff --git i/src/qemu/qemu_capabilities.h w/src/qemu/qemu_capabilities.h
> index 8057479..a4c9cfd 100644
> --- i/src/qemu/qemu_capabilities.h
> +++ w/src/qemu/qemu_capabilities.h
> @@ -83,6 +83,7 @@ enum qemuCapsFlags {
>      QEMUD_CMD_FLAG_SPICE         = (1LL << 46), /* Is -spice avail */
>      QEMUD_CMD_FLAG_VGA_NONE      = (1LL << 47), /* The 'none' arg for
> '-vga' */
>      QEMUD_CMD_FLAG_MIGRATE_QEMU_FD = (1LL << 48), /* -incoming fd:n */
> +    QEMUD_CMD_FLAG_HDA_DUPLEX    = (1LL << 49), /* -device hda-duplex */
>  };
> 
>  virCapsPtr qemuCapsInit(virCapsPtr old_caps);
> 
> 
> and issue a much nicer
> 
> +        if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HDA_DUPLEX)) {
> +            qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                            _("this QEMU binary lacks hda support"));
> +            goto error;
> 
> 
>>
>> 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)
> 
> I agree with erroring out, but the question is whether it is nicer to
> rely on qemu erroring out or doing it ourselves, when it is not that
> much more work to do it ourselves.
> 

You just won't take 'lazy' for an answer will you? :) Thanks for the code,
I'll fold this into the next posting.

- Cole