[RFC PATCH v2] fix error message in virMigrate3 if connection is broken
Nikolay Shirokovskiy
nshirokovskiy at virtuozzo.com
Thu Oct 15 15:58:59 UTC 2020
Polite ping
On 24.09.2020 11:54, Nikolay Shirokovskiy wrote:
> Changes from v1 [1]:
> - return error value from VIR_DRV_SUPPORTS_FEATURE instead
> of setting error to out argument (decrease over enginering
> level on patch generator in other words :)
>
> 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. Let's return error from macro as is.
>
> I guess all the other occurences of VIR_DRV_SUPPORTS_FEATURE need to be updated
> as well so that we detect errors early and not have issues similar to
> virDomainMigrate3. I'm going to fix the other places if this RFC is approved.
>
> [1] https://www.redhat.com/archives/libvir-list/2020-September/msg01056.html
> ---
> src/driver.h | 14 ++++++++
> src/libvirt-domain.c | 91 +++++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 79 insertions(+), 26 deletions(-)
>
> diff --git a/src/driver.h b/src/driver.h
> index 6278aa0..c29b2fa 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -60,6 +60,20 @@ typedef enum {
> (drv)->connectSupportsFeature((conn), (feature)) > 0 : 0)
>
>
> +/*
> + * A little wrapper for connectSupportsFeature API to test that the API
> + * itself is available first. Return values are same as for API.
> + *
> + * Returns:
> + * -1 on error
> + * 0 feature is not supported
> + * 1 feature is supported
> + */
> +#define VIR_DRV_SUPPORTS_FEATURE2(drv, conn, feature) \
> + ((drv)->connectSupportsFeature ? \
> + (drv)->connectSupportsFeature((conn), (feature)) : 0)
> +
> +
> #define __VIR_DRIVER_H_INCLUDES___
>
> #include "driver-hypervisor.h"
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index cde86c7..03c357f 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3846,6 +3846,9 @@ virDomainMigrate3(virDomainPtr domain,
> const char *dname = NULL;
> const char *dxml = NULL;
> unsigned long long bandwidth = 0;
> + int rc_src;
> + int rc_dst;
> + int rc;
>
> VIR_DOMAIN_DEBUG(domain, "dconn=%p, params=%p, nparms=%u flags=0x%x",
> dconn, params, nparams, flags);
> @@ -3878,15 +3881,21 @@ virDomainMigrate3(virDomainPtr domain,
> }
>
> if (flags & VIR_MIGRATE_OFFLINE) {
> - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
> + rc = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_OFFLINE);
> + if (rc < 0) {
> + goto error;
> + } else if (rc == 0) {
> 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)) {
> + rc = VIR_DRV_SUPPORTS_FEATURE2(dconn->driver, dconn,
> + VIR_DRV_FEATURE_MIGRATION_OFFLINE);
> + if (rc < 0) {
> + goto error;
> + } else if (rc == 0) {
> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("offline migration is not supported by "
> "the destination host"));
> @@ -3899,21 +3908,30 @@ virDomainMigrate3(virDomainPtr domain,
> * the flag for just the source side. We mask it out to allow
> * 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"));
> - goto error;
> + if (flags & VIR_MIGRATE_CHANGE_PROTECTION) {
> + rc = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION);
> + if (rc < 0) {
> + goto error;
> + } else if (rc == 0) {
> + 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)) {
> + rc_src = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_PARAMS);
> + if (rc_src < 0)
> + goto error;
> + rc_dst = VIR_DRV_SUPPORTS_FEATURE2(dconn->driver, dconn,
> + VIR_DRV_FEATURE_MIGRATION_PARAMS);
> + if (rc_dst < 0)
> + goto error;
> + if (rc_src && rc_dst) {
> VIR_DEBUG("Using migration protocol 3 with extensible parameters");
> ddomain = virDomainMigrateVersion3Params(domain, dconn, params,
> nparams, flags);
> @@ -3939,17 +3957,30 @@ 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)) {
> + rc_src = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_V3);
> + if (rc_src < 0)
> + goto error;
> + rc_dst = VIR_DRV_SUPPORTS_FEATURE2(dconn->driver, dconn,
> + VIR_DRV_FEATURE_MIGRATION_V3);
> + if (rc_dst < 0)
> + goto error;
> + if (rc_src && rc_dst) {
> 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)) {
> + goto done;
> + }
> +
> + rc_src = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_V2);
> + if (rc_src < 0)
> + goto error;
> + rc_dst = VIR_DRV_SUPPORTS_FEATURE2(dconn->driver, dconn,
> + VIR_DRV_FEATURE_MIGRATION_V2);
> + if (rc_dst < 0)
> + goto error;
> + if (rc_src && rc_dst) {
> VIR_DEBUG("Using migration protocol 2");
> if (dxml) {
> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> @@ -3959,10 +3990,18 @@ 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)) {
> + goto done;
> + }
> +
> + rc_src = VIR_DRV_SUPPORTS_FEATURE2(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_V1);
> + if (rc_src < 0)
> + goto error;
> + rc_dst = VIR_DRV_SUPPORTS_FEATURE2(dconn->driver, dconn,
> + VIR_DRV_FEATURE_MIGRATION_V1);
> + if (rc_dst < 0)
> + goto error;
> + if (rc_src && rc_dst) {
> VIR_DEBUG("Using migration protocol 1");
> if (dxml) {
> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>
More information about the libvir-list
mailing list