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&gt; > --- > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list