On 12/28/2013 11:11 AM, Eric Blake wrote:
We weren't very consistent in our use of VIR_ERR_NO_SUPPORT; many users just passed __FUNCTION__ on, while others passed "%s" to silence over-eager compilers that warn about __FUNCTION__ not containing any %. It's nicer to route all these uses through a single macro, so that if we ever need to change the reporting, we can do it in one place.
I verified that 'virsh -c test:///default qemu-monitor-command test foo' gives the same error message before and after this patch: error: this function is not supported by the connection driver: virDomainQemuMonitorCommand
Note that in libvirt.c, we were inconsistent on whether virDomain* API used virLibConnError() (with VIR_FROM_NONE) or virLibDomainError() (with VIR_FROM_DOMAIN); this patch unifies these errors to all use VIR_FROM_NONE, on the grounds that it is unlikely that a caller learning that a call is unimplemented can do anything in particular with extra knowledge of which error domain it belongs to.
* src/util/virerror.h (virReportUnsupportedError): New macro. * src/libvirt.c: Use it. * src/libvirt-qemu.c: Likewise. * src/libvirt-lxc.c: Likewise. * src/lxc/lxc_driver.c: Likewise. * src/security/security_manager.c: Likewise. * src/util/virinitctl.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt-lxc.c | 2 +- src/libvirt-qemu.c | 7 +- src/libvirt.c | 612 ++++++++++++++++++++-------------------- src/lxc/lxc_driver.c | 2 +- src/security/security_manager.c | 46 +-- src/util/virerror.h | 5 + src/util/virinitctl.c | 4 +- src/xen/xen_driver.c | 8 +- 8 files changed, 345 insertions(+), 341 deletions(-)
Thus all error messages of this type will start with "libvirt: Unknown" rather than "some" starting with "libvirt: <domain-name>". I'm on the fence whether this is OK/desired. On one hand - it simplifies things and really API's aren't necessarily tied to domains. However, for applications that have a list of domains to try calling the same domain function that "has" been reporting the domain name upon return and checking if the domain is listed as not supporting a particular function, then this could cause a regression for them. Of course they'd have to have found one of the API's and they'd have to check the error message. Of course then there's the tester that creates a domain named "Unknown" that'll really be confused :-) The list of 23 API's in libvirt.c that would now use "Unknown" rather than the real name would be: virDomainMigratePerform Begin3 Perform3 Confirm3 Begin3Params Perform3Params Confirm3Params virDomainBlockStats InterfaceStats MemoryStats BlockPeek BlockResize MemoryPeek BlockJobAbort BlockJobInfo BlockJobSetSpeed BlockPull BlockRebase BlockCommit virDomainOpenGraphics <- Different than rest in the way it's checked SetBlockIoTune GetBlockIoTune GetCPUStats The only one that I'd say is different is virDomainOpenGraphics(). It checks VIR_DRV_SUPPORTS_FEATURE on one of its calls to virLibDomainError(). Thus perhaps it'd be better to generate a "real" error so as to differentiate between the function not being available as a general rule of thumb as opposed to it not being available to a specific domain because the domain doesn't support a specific feature. In this case VIR_DRV_FEATURE_FD_PASSING supported in the driver. I'm OK with an ACK - I just wanted to provide the "counter point" though to why a caller may want to know the domain name of an unimplemented function. Python makes it all too easy to snag error messages and make decisions based on "known" fields. John