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
