| Message ID | 1421171432-8733-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/13/2015 10:50 AM, Markus Armbruster wrote: > Commit 7572150 adopted QERR_DEVICE_NOT_ACTIVE for the purpose, > probably because adding another error seemed cumbersome overkill. > Produces "No spice device has been activated", which is awkward. > > We've since abandoned our quest for "rich" error objects. Time to > undo the damage to this error message. Replace it by "SPICE is not in > use". > > Keep the stupid DeviceNotActive ErrorClass for compatibility, even > though Libvirt doesn't use it. Libvirt has a few places that look explicitly for "DeviceNotActive" when deciding how to handle failure reporting; I didn't audit closely enough to see if it actually changes behavior if the class were to disappear. Your approach in this patch is safest, at any rate. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > include/ui/qemu-spice.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 01/13/2015 10:50 AM, Markus Armbruster wrote: >> Commit 7572150 adopted QERR_DEVICE_NOT_ACTIVE for the purpose, >> probably because adding another error seemed cumbersome overkill. >> Produces "No spice device has been activated", which is awkward. >> >> We've since abandoned our quest for "rich" error objects. Time to >> undo the damage to this error message. Replace it by "SPICE is not in >> use". >> >> Keep the stupid DeviceNotActive ErrorClass for compatibility, even >> though Libvirt doesn't use it. > > Libvirt has a few places that look explicitly for "DeviceNotActive" when > deciding how to handle failure reporting; I didn't audit closely enough > to see if it actually changes behavior if the class were to disappear. I studied libvirt's use of error classes other than GenericError before my Christmas break. My findings: * CommandNotFound: used; libvirt could perhaps rely on query-commands instead * DeviceEncrypted: not used (surprise, surprise) * DeviceNotActive: used with balloon and block jobs, not used with spice * DeviceNotFound: used to detect active commit support, with device-list-properties and with qom-list (which isn't about devices at all) * KVMMissingCap: used; occurs only with balloon, and it shandled just like DeviceNotActive > Your approach in this patch is safest, at any rate. I'm resigned to keeping these error classes around. I'm still fighting their proliferation to new code. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> include/ui/qemu-spice.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h index b3f2679..99bb622 100644 --- a/include/ui/qemu-spice.h +++ b/include/ui/qemu-spice.h @@ -91,8 +91,8 @@ static inline int qemu_spice_display_add_client(int csock, int skipauth, static inline int qemu_using_spice(Error **errp) { if (!using_spice) { - /* correct one? spice isn't a device ,,, */ - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, + "SPICE is not in use"); return 0; } return 1;
Commit 7572150 adopted QERR_DEVICE_NOT_ACTIVE for the purpose, probably because adding another error seemed cumbersome overkill. Produces "No spice device has been activated", which is awkward. We've since abandoned our quest for "rich" error objects. Time to undo the damage to this error message. Replace it by "SPICE is not in use". Keep the stupid DeviceNotActive ErrorClass for compatibility, even though Libvirt doesn't use it. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/ui/qemu-spice.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)