[libvirt] [PATCH 08/24] maint: improve VIR_ERR_NO_SUPPORT usage

[John Ferlan]

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:

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