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.