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

[John Ferlan]

On 01/02/2014 04:17 PM, John Ferlan wrote:
> 
> 
> 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>".

hmm.. after re-reading virRaiseErrorFull(), virDefaultErrorFunc(), and
friends - seems my eyes weren't functioning with my brain completely
with regard to error domain and domain domain...  I'm less concerned now
with my original thoughts.  Guess some of the work recently in virt-test
has made me a bit more sensitive to changing error messages as believe
it or not, they are used to make decisions in some tests to FAIL or SKIP
based on whether some functionality exists.  For example, if hotplug
isn't supported an error is returned "cannot change vcpu count of this
domain" - and that's a decision point for returning SKIP or FAIL.

John
> 
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>