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

[Nikolay Shirokovskiy]

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