[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] RFC: fix error message in virMigrate3 if connection is broken



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]