Re: [libvirt] [PATCH 08/24] maint: improve VIR_ERR_NO_SUPPORT usage
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(a)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