On 23.09.2020 22:50, Jiri Denemark wrote:
On Mon, Sep 21, 2020 at 10:51:16 +0300, Nikolay Shirokovskiy wrote:
Currently virDomainMigrate3 reports "this function is not supported by the connection driver: virDomainMigrate3" if connection to destination for example is broken. This is a bit misleading.
This is a result of we treat errors as unsupported feature in VIR_DRV_SUPPORTS_FEATURE macro. So let's handle errors instead. In order to keep logic clean as before there are new helper functions virDrvSupportsFeature/virDrvNSupportsFeature. They allow to keep if/else if structure of feature tests.
I guess all the other occurences of VIR_DRV_SUPPORTS_FEATURE need to be replaced with one these new helper functions so that we detect error early and not have issues similar to virDomainMigrate3. I'm going to fix the other places if this RFC is approved.
To be honest, I think this is quite ugly.
--- src/datatypes.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++ src/datatypes.h | 7 +++++ src/libvirt-domain.c | 76 +++++++++++++++++++++++++++++++--------------------- 3 files changed, 123 insertions(+), 30 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index 1db38c5..3fb71f4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -1257,3 +1257,73 @@ virAdmClientDispose(void *obj)
virObjectUnref(clt->srv); } + + +/* + * Tests if feature is supported by connection. If testing failed + * due to error then function returns true as well and set @err flag + * to true. Thus positive result should be checked for an error. + * If @err is already set to true then no checking is done and + * the function returns true immediately so that previous error + * is not overwritten. + * + * Returns: + * true feature is supported or testing hit error + * false feature is not supported + */ +bool +virDrvSupportsFeature(virConnectPtr conn, + virDrvFeature feature, + bool *err) +{ + int rc; + + if (*err) + return true; + + if (!conn->driver->connectSupportsFeature) + return false; + + if ((rc = conn->driver->connectSupportsFeature(conn, feature)) < 0) { + *err = true; + return true; + } + + return rc > 0 ? true : false; +}
I would just make virDrvSupportsFeature a tiny wrapper around conn->driver->connectSupportsFeature checking conn->driver->connectSupportsFeature != NULL and that's it. It could break the if/elseif flow, but with much cleaner semantics and reduced black magic.
Ok, thanx. I'll send next version. Nikolay