[PATCH] RFC: fix error message in virMigrate3 if connection is broken
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. --- 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; +} + + +/* + * Tests if feature is NOT 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. + * + * Note that this function is not merge negation of virDrvSupportsFeature. + * + * Returns: + * true feature is not supported or testing hit error + * false feature is supported + */ +bool +virDrvNSupportsFeature(virConnectPtr conn, + virDrvFeature feature, + bool *err) +{ + int rc; + + if (*err) + return true; + + if (!conn->driver->connectSupportsFeature) + return true; + + if ((rc = conn->driver->connectSupportsFeature(conn, feature)) < 0) { + *err = true; + return true; + } + + return rc > 0 ? false : true; +} diff --git a/src/datatypes.h b/src/datatypes.h index ade3779..e468d92 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -871,3 +871,10 @@ int virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr cbd virFreeCallback freecb); int virAdmConnectCloseCallbackDataUnregister(virAdmConnectCloseCallbackDataPtr cbdata, virAdmConnectCloseFunc cb); + +bool virDrvSupportsFeature(virConnectPtr conn, + virDrvFeature feature, + bool *err); +bool virDrvNSupportsFeature(virConnectPtr conn, + virDrvFeature feature, + bool *err); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cde86c7..f4f626a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3846,6 +3846,7 @@ virDomainMigrate3(virDomainPtr domain, const char *dname = NULL; const char *dxml = NULL; unsigned long long bandwidth = 0; + bool err = false; VIR_DOMAIN_DEBUG(domain, "dconn=%p, params=%p, nparms=%u flags=0x%x", dconn, params, nparams, flags); @@ -3878,18 +3879,20 @@ virDomainMigrate3(virDomainPtr domain, } if (flags & VIR_MIGRATE_OFFLINE) { - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_OFFLINE)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the source host")); + if (virDrvNSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE, &err)) { + if (!err) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the source host")); goto error; } - if (!VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_OFFLINE)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the destination host")); + if (virDrvNSupportsFeature(dconn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE, &err)) { + if (!err) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the destination host")); goto error; } } @@ -3900,20 +3903,25 @@ virDomainMigrate3(virDomainPtr domain, * migration from newer source to an older destination that * rejects the flag. */ if (flags & VIR_MIGRATE_CHANGE_PROTECTION && - !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("cannot enforce change protection")); + virDrvNSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION, + &err)) { + if (!err) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("cannot enforce change protection")); goto error; } flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; /* Prefer extensible API but fall back to older migration APIs if params * only contains parameters which were supported by the older API. */ - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_PARAMS) && - VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_PARAMS)) { + if (virDrvSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATION_PARAMS, &err) && + virDrvSupportsFeature(dconn, + VIR_DRV_FEATURE_MIGRATION_PARAMS, &err)) { + if (err) + goto error; + VIR_DEBUG("Using migration protocol 3 with extensible parameters"); ddomain = virDomainMigrateVersion3Params(domain, dconn, params, nparams, flags); @@ -3939,17 +3947,22 @@ virDomainMigrate3(virDomainPtr domain, goto error; } - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3) && - VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_V3)) { + if (virDrvSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3, &err) && + virDrvSupportsFeature(dconn, VIR_DRV_FEATURE_MIGRATION_V3, &err)) { + if (err) + goto error; + VIR_DEBUG("Using migration protocol 3"); ddomain = virDomainMigrateVersion3(domain, dconn, dxml, flags, dname, uri, bandwidth); - } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V2) && - VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_V2)) { + } else if (virDrvSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATION_V2, &err) && + virDrvSupportsFeature(dconn, + VIR_DRV_FEATURE_MIGRATION_V2, &err)) { + if (err) + goto error; + VIR_DEBUG("Using migration protocol 2"); if (dxml) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", @@ -3959,10 +3972,13 @@ virDomainMigrate3(virDomainPtr domain, } ddomain = virDomainMigrateVersion2(domain, dconn, flags, dname, uri, bandwidth); - } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V1) && - VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_V1)) { + } else if (virDrvSupportsFeature(domain->conn, + VIR_DRV_FEATURE_MIGRATION_V1, &err) && + virDrvSupportsFeature(dconn, + VIR_DRV_FEATURE_MIGRATION_V1, &err)) { + if (err) + goto error; + VIR_DEBUG("Using migration protocol 1"); if (dxml) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", -- 1.8.3.1
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
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
participants (2)
-
Jiri Denemark -
Nikolay Shirokovskiy