[libvirt] [PATCH 08/24] maint: improve VIR_ERR_NO_SUPPORT usage
jferlan at redhat.com
Thu Jan 2 21:17:01 UTC 2014
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 at 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:
virDomainOpenGraphics <- Different than rest in the way it's checked
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.
More information about the libvir-list