Bug 669524
Summary: | Confusing error message from -device <unknown dev> | ||
---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | Cole Robinson <crobinso> |
Component: | qemu-kvm | Assignee: | Markus Armbruster <armbru> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Virtualization Bugs <virt-bugs> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | 7.0 | CC: | areis, armbru, crobinso, juli, juzhang, lihuang, mkenneth, qiguo, shuang, sluo, tburke, virt-maint |
Target Milestone: | rc | ||
Target Release: | 7.0 | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | qemu-kvm-1.5.3-39.el7 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2014-06-13 09:43:42 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 580953 |
Description
Cole Robinson
2011-01-13 21:31:52 UTC
"-device foobar" is short for "-device driver=foobar". The error message makes sense then. Once again, syntactic sugar leads to suboptimal error messages. Several other options provide similar sugar (QemuOptsList member implied_opt_name). Not sure this is fixable with reasonable effort. An upstream fix would probably mess with three subsystems: qdev, QemuOpts and QMP. All three are going through major change. Anthony promises the change will lead to better error handling. Now, I'll believe that when I see it, but regardless, I'd rather not interfere without a really compelling reason. Which makes me want to WONTFIX this bug. But: "A user can easily encounter this via libvirt" makes me hesistate. What's the user experience exactly? If improving the user experience turns out to be important, maybe improving it in libvirt is more practical at this time. Would it be possible for libvirt to catch the suboptimal error and report it in a friendlier way? Alternatively, would it be possible for libvirt to query available devices with -device \?, and simply not ask for devices that aren't available? (In reply to comment #4) > An upstream fix would probably mess with three subsystems: qdev, QemuOpts and > QMP. All three are going through major change. Anthony promises the change > will lead to better error handling. Now, I'll believe that when I see it, but > regardless, I'd rather not interfere without a really compelling reason. Which > makes me want to WONTFIX this bug. > > But: "A user can easily encounter this via libvirt" makes me hesistate. What's > the user experience exactly? > If someone uses a new libvirt with an older qemu, and specifies some new device in the guest XML and libvirt just passes it through to qemu, guest startup fails with the reported error message. > If improving the user experience turns out to be important, maybe improving it > in libvirt is more practical at this time. > > Would it be possible for libvirt to catch the suboptimal error and report it in > a friendlier way? > Sure, but that sounds like a hack and not future proof at all. And if libvirt can catch the error and scrape it to report a more accurate meaning, QEMU should be able to as well. > Alternatively, would it be possible for libvirt to query available devices with > -device \?, and simply not ask for devices that aren't available? Sure, but that is more moving parts and could cause false positives, not specifying a device that could work if our parsing failed (or regressed between qemu versions). I know there is QOM stuff going on upstream, and since this is targeted for RHEL7 and not RHEL6 it sounds like now is a good opportunity to get this sorted out upstream. But if it's still programmaticly infeasible to address qemu for RHEL7 I'd say just close this bug WONTFIX. Bug has been punted from 6.1 to 7.0 since my NEEDINFO. Your reply suggests you're fine with that. Let's try to get decent error messages out of upstream QOM in time for RHEL-7. Thanks! *** Bug 892286 has been marked as a duplicate of this bug. *** Current upstream's error message: -device foobar: Parameter 'driver' expects device type Two years after comment#4, much has changed in the code, yet my assessment remains unchanged: we'd still have to touch several subsystems, which are still very much in flux, and the effort required to fix the bug seems very hard to justify by the bug's impact on users. Comment#6 made the case against leaving the bug alone, and against libvirt working around it, in four steps: 1. The bug impacts users: a scenario exists where users can trip this error with libvirt, and the message they then get is confusing. 2. Catching the error in libvirt so we can emit a better message would require an unpleasant and fragile hack. 3. Avoiding the error in libvirt would require libvirt gaining precise knowledge on available devices, which is complex and possibly fragile. 4. Therefore, the only option is to improve the message at its root, in qemu-kvm. Since then, we've acquired means to reliably and robustly query available device types in QMP: { "execute": "qom-list-types", "arguments": { "implements": "device" } } I think we should reassess 3 now. Cole? How about this? diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index c30c2f6..ded5b3e 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -145,7 +145,7 @@ void assert_no_error(Error *err); ERROR_CLASS_GENERIC_ERROR, "Invalid parameter type for '%s', expected: %s" #define QERR_INVALID_PARAMETER_VALUE \ - ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' expects %s" + ERROR_CLASS_GENERIC_ERROR, "Invalid '%s' value '%s'" #define QERR_INVALID_PASSWORD \ ERROR_CLASS_GENERIC_ERROR, "Password incorrect" diff --git a/qdev-monitor.c b/qdev-monitor.c index 410cdcb..85756f7 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -468,7 +468,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) } if (!obj) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); + qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", driver); return NULL; } That would require fixing up the rest of the usages of QERR_INVALID_PARAMATER_VALUE but there aren't many. The string error message of 'Paramater FOO requires HUMAN STRING DESCRIBING WHAT FOO IS' can be actively misleading anyways. AC97 is a valid device type, but if it's compiled out, we give a confusing error 'paramater 'driver' expects device type'. Your suggestion of handling this in libvirt sounds reasonable and could be useful for other libvirt enhancements as well, but you'll need to ask the libvirt guys about that. I originally filed this bug with the assumption that the fix was trivial-ish, if my above suggestion isn't sufficient and there isn't an equally simple fix, then I'd just recommend closing it as WONTFIX rather than have it linger forever. QERR_INVALID_PARAMETER_VALUE is a product of upstream's attempt at a rich error reporting API. Errors were classified into classes such as InvalidParameterValue, and a single human-readable error message was associated with each class. Unsurprisingly, that led to rather unhelpful error messages when the same class of error was reported from many different contexts. QERR_INVALID_PARAMETER_VALUE is such a case. Upstream eventually recognized that the rich error reporting API was a mistake, and switched to a simpler and saner one. We still have the old error macros like QERR_INVALID_PARAMETER_VALUE spread all over the code, printing the same, old, unhelpful error messages. Your proposed change to the wording of the error message associated with QERR_INVALID_PARAMETER_VALUE is problematic. It is not obvious that the new text is an improvement in all the contexts QERR_INVALID_PARAMETER_VALUE is used. In particular since old message gives a hint at what's expected, while the new one does not. Perhaps we can change just the single use, like this: if (!obj) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type"); + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "%s is not a valid device model name", driver); return NULL; } The message text may need more polish; I'm just illustrating how to have another message here without changing messages elsewhere. Proposed upstream patch: https://lists.nongnu.org/archive/html/qemu-devel/2013-12/msg03670.html Fix included in qemu-kvm-1.5.3-39.el7 Fix also fixes bug 1026712 and bug 1052085. QE, please add their reproducers to your testing. They are: From bug 1026712: Steps to Reproduce: 1. Start with -device virtio-pci Actual results: Assertion failure Expected results: Fatal error "qemu-kvm: -device virtio-pci: Parameter 'driver' expects non-abstract device type" From bug 1052085: Steps to Reproduce: 1. qemu-kvm -nodefaults -S -display none -monitor stdio 2. device_add rng-egd Actual results: Assertion failure Expected results: Command fails with error message "'rng-egd' is not a valid device model name" See these bugs for additional detail. *** Bug 1026712 has been marked as a duplicate of this bug. *** *** Bug 1052085 has been marked as a duplicate of this bug. *** *** Bug 1055025 has been marked as a duplicate of this bug. *** Reproduced this issue with the same steps as comment #0 and comment #18 on qemu-kvm-1.5.3-38.el7.x86_64. host info: # uname -r && rpm -q qemu-kvm 3.10.0-76.el7.x86_64 qemu-kvm-1.5.3-38.el7.x86_64 Steps and Results: From bug 669524: # /usr/libexec/qemu-kvm -device foobar Segmentation fault (core dumped) From bug 1026712: # /usr/libexec/qemu-kvm -device virtio-pci qemu-kvm: -device virtio-pci: Parameter 'driver' expects non-abstract device type From bug 1052085: # /usr/libexec/qemu-kvm -nodefaults -S -display none -monitor stdio QEMU 1.5.3 monitor - type 'help' for more information (qemu) device_add rng-egd qdev-monitor.c:486:qdev_device_add: Object 0x7f76d76f31e0 is not an instance of type device Aborted (core dumped) Verified this issue on qemu-kvm-1.5.3-45.el7.x86_64. host info: # uname -r && rpm -q qemu-kvm 3.10.0-76.el7.x86_64 qemu-kvm-1.5.3-45.el7.x86_64 Steps and Results: From bug 669524: # /usr/libexec/qemu-kvm -device foobar qemu-kvm: -device foobar: 'foobar' is not a valid device model name From bug 1026712: # /usr/libexec/qemu-kvm -device virtio-pci qemu-kvm: -device virtio-pci: Parameter 'driver' expects non-abstract device type From bug 1052085: # /usr/libexec/qemu-kvm -nodefaults -S -display none -monitor stdio QEMU 1.5.3 monitor - type 'help' for more information (qemu) device_add rng-egd 'rng-egd' is not a valid device model name (qemu) q Base on above, this issue has been fixed correctly, move to VERIFIED status. Best Regards, sluo This request was resolved in Red Hat Enterprise Linux 7.0. Contact your manager or support representative in case you have further questions about the request. |