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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list