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

[Eric Blake]

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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110114/e5e79844/attachment-0001.sig>