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", >
