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.
Jirka
