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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list