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

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.