[PATCH v3 0/4] handle error in feature testing
Justification is in the last patch. Diff to v2 [1]: - instead of RFC where fix was applied only to virMigrate3 now all the places where VIR_DRV_SUPPORTS_FEATURE is used patched to handle errors [1] [RFC PATCH v2] fix error message in virMigrate3 if connection is broken https://www.redhat.com/archives/libvir-list/2020-September/msg01348.html Nikolay Shirokovskiy (4): src: adopt to VIR_DRV_SUPPORTS_FEATURE return -1 libxl: adopt to VIR_DRV_SUPPORTS_FEATURE return -1 qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1 src: don't hide error in VIR_DRV_SUPPORTS_FEATURE src/driver.h | 9 +- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++-------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- src/libxl/libxl_migration.c | 10 +- src/qemu/qemu_migration.c | 33 ++- 6 files changed, 396 insertions(+), 192 deletions(-) -- 1.8.3.1
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++++++----------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- 3 files changed, 365 insertions(+), 171 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5edc73e..2f9081a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2130,6 +2130,7 @@ virDomainGetMemoryParameters(virDomainPtr domain, int *nparams, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x", params, (nparams) ? *nparams : -1, flags); @@ -2142,8 +2143,11 @@ virDomainGetMemoryParameters(virDomainPtr domain, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, @@ -2251,6 +2255,7 @@ virDomainGetNumaParameters(virDomainPtr domain, int *nparams, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x", params, (nparams) ? *nparams : -1, flags); @@ -2263,8 +2268,11 @@ virDomainGetNumaParameters(virDomainPtr domain, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = domain->conn; @@ -2369,6 +2377,7 @@ virDomainGetBlkioParameters(virDomainPtr domain, int *nparams, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x", params, (nparams) ? *nparams : -1, flags); @@ -2381,8 +2390,11 @@ virDomainGetBlkioParameters(virDomainPtr domain, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, @@ -2851,12 +2863,14 @@ virDomainMigrateVersion2(virDomainPtr domain, return NULL; } - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_XML_MIGRATABLE)) { + ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_XML_MIGRATABLE); + if (ret < 0) + return NULL; + if (ret) getxml_flags |= VIR_DOMAIN_XML_MIGRATABLE; - } else { + else getxml_flags |= VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_UPDATE_CPU; - } dom_xml = domain->conn->driver->domainGetXMLDesc(domain, getxml_flags); if (!dom_xml) @@ -3005,8 +3019,11 @@ virDomainMigrateVersion3Full(virDomainPtr domain, return NULL; params = tmp; - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) + ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (ret < 0) + return NULL; + if (ret) protection = VIR_MIGRATE_CHANGE_PROTECTION; VIR_DEBUG("Begin3 %p", domain->conn); @@ -3403,6 +3420,8 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, int nparams, unsigned int flags) { + int rc; + VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparams=%d, flags=0x%x", NULLSTR(dconnuri), params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); @@ -3411,19 +3430,28 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain, virDomainMigrateCheckNotLocal(dconnuri) < 0) return -1; - if ((flags & VIR_MIGRATE_PEER2PEER) && - VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_PARAMS)) { - VIR_DEBUG("Using migration protocol 3 with extensible parameters"); - if (!domain->conn->driver->domainMigratePerform3Params) { - virReportUnsupportedError(); + if (flags & VIR_MIGRATE_PEER2PEER) { + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_PARAMS); + if (rc < 0) return -1; + if (rc) { + VIR_DEBUG("Using migration protocol 3 with extensible parameters"); + if (!domain->conn->driver->domainMigratePerform3Params) { + virReportUnsupportedError(); + return -1; + } + return domain->conn->driver->domainMigratePerform3Params + (domain, dconnuri, params, nparams, + NULL, 0, NULL, NULL, flags); } - return domain->conn->driver->domainMigratePerform3Params - (domain, dconnuri, params, nparams, - NULL, 0, NULL, NULL, flags); - } else if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3)) { + } + + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + if (rc < 0) + return -1; + if (rc) { VIR_DEBUG("Using migration protocol 3"); if (!domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); @@ -3515,6 +3543,9 @@ virDomainMigrate(virDomainPtr domain, unsigned long bandwidth) { virDomainPtr ddomain = NULL; + int rc_src; + int rc_dst; + int rc; VIR_DOMAIN_DEBUG(domain, "dconn=%p, flags=0x%lx, dname=%s, uri=%s, bandwidth=%lu", @@ -3539,25 +3570,33 @@ virDomainMigrate(virDomainPtr domain, error); 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")); + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + 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)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the destination host")); + + rc = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the destination host")); goto error; } } if (flags & VIR_MIGRATE_PEER2PEER) { - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_P2P)) { + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P); + if (rc < 0) + goto error; + if (rc) { g_autofree char *dstURI = NULL; if (uri == NULL) { dstURI = virConnectGetURI(dconn); @@ -3582,12 +3621,15 @@ virDomainMigrate(virDomainPtr domain, * the flag for just the source side. We mask it out for * non-peer2peer 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("cannot enforce change protection")); + goto error; + } } flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; if (flags & VIR_MIGRATE_TUNNELLED) { @@ -3597,34 +3639,57 @@ virDomainMigrate(virDomainPtr domain, } /* Check that migration is supported by both drivers. */ - 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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, NULL, 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V2); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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"); 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V1); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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"); ddomain = virDomainMigrateVersion1(domain, dconn, flags, dname, uri, bandwidth); - } else { - /* This driver does not support any migration method */ - virReportUnsupportedError(); - goto error; + goto done; } + + /* This driver does not support any migration method */ + virReportUnsupportedError(); + goto error; } + done: if (ddomain == NULL) goto error; @@ -3671,6 +3736,9 @@ virDomainMigrate2(virDomainPtr domain, unsigned long bandwidth) { virDomainPtr ddomain = NULL; + int rc_src; + int rc_dst; + int rc; VIR_DOMAIN_DEBUG(domain, "dconn=%p, flags=0x%lx, dname=%s, uri=%s, bandwidth=%lu", @@ -3695,25 +3763,32 @@ virDomainMigrate2(virDomainPtr domain, error); 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")); + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + 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)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the destination host")); + rc = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the destination host")); goto error; } } if (flags & VIR_MIGRATE_PEER2PEER) { - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_P2P)) { + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P); + if (rc < 0) + goto error; + if (rc) { g_autofree char *dstURI = virConnectGetURI(dconn); if (!dstURI) return NULL; @@ -3735,12 +3810,15 @@ virDomainMigrate2(virDomainPtr domain, * the flag for just the source side. We mask it out for * non-peer2peer 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("cannot enforce change protection")); + goto error; + } } flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; if (flags & VIR_MIGRATE_TUNNELLED) { @@ -3750,17 +3828,30 @@ virDomainMigrate2(virDomainPtr domain, } /* Check that migration is supported by both drivers. */ - 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V2); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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", @@ -3769,10 +3860,18 @@ virDomainMigrate2(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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V1); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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", @@ -3781,13 +3880,15 @@ virDomainMigrate2(virDomainPtr domain, } ddomain = virDomainMigrateVersion1(domain, dconn, flags, dname, uri, bandwidth); - } else { - /* This driver does not support any migration method */ - virReportUnsupportedError(); - goto error; + goto done; } + + /* This driver does not support any migration method */ + virReportUnsupportedError(); + goto error; } + done: if (ddomain == NULL) goto error; @@ -3846,6 +3947,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,18 +3982,23 @@ 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")); + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + 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)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("offline migration is not supported by " - "the destination host")); + + rc = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the destination host")); goto error; } } @@ -3899,21 +4008,29 @@ 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (rc <= 0) { + 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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_PARAMS); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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 +4056,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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V2); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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 +4089,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_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V1); + if (rc_src < 0) + goto error; + rc_dst = VIR_DRV_SUPPORTS_FEATURE(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", @@ -3972,12 +4110,13 @@ virDomainMigrate3(virDomainPtr domain, } ddomain = virDomainMigrateVersion1(domain, dconn, flags, dname, uri, bandwidth); - } else { - /* This driver does not support any migration method */ - virReportUnsupportedError(); - goto error; + goto done; } + /* This driver does not support any migration method */ + virReportUnsupportedError(); + goto error; + done: if (ddomain == NULL) goto error; @@ -3994,33 +4133,44 @@ static int virDomainMigrateUnmanagedCheckCompat(virDomainPtr domain, unsigned int flags) { + int rc; + VIR_EXCLUSIVE_FLAGS_RET(VIR_MIGRATE_NON_SHARED_DISK, VIR_MIGRATE_NON_SHARED_INC, -1); - if (flags & VIR_MIGRATE_OFFLINE && - !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")); - return -1; + if (flags & VIR_MIGRATE_OFFLINE) { + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_OFFLINE); + + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("offline migration is not supported by " + "the source host")); + return -1; + } } if (flags & VIR_MIGRATE_PEER2PEER) { - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_P2P)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("p2p migration is not supported by " - "the source host")); + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_P2P); + + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("p2p migration is not supported by " + "the source host")); return -1; } } else { - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_DIRECT)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("direct migration is not supported by " - "the source host")); + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_DIRECT); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("direct migration is not supported by " + "the source host")); return -1; } } @@ -5201,6 +5351,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p, flags=0x%x", params, nparams, flags); @@ -5213,8 +5364,11 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, virCheckNonNullArgGoto(nparams, error); virCheckPositiveArgGoto(*nparams, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, @@ -5465,6 +5619,7 @@ virDomainBlockStatsFlags(virDomainPtr dom, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(dom, "disk=%s, params=%p, nparams=%d, flags=0x%x", disk, params, nparams ? *nparams : -1, flags); @@ -5478,8 +5633,11 @@ virDomainBlockStatsFlags(virDomainPtr dom, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = dom->conn; @@ -5657,6 +5815,7 @@ virDomainGetInterfaceParameters(virDomainPtr domain, int *nparams, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "device=%s, params=%p, nparams=%d, flags=0x%x", device, params, (nparams) ? *nparams : -1, flags); @@ -5669,8 +5828,11 @@ virDomainGetInterfaceParameters(virDomainPtr domain, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = domain->conn; @@ -10581,6 +10743,8 @@ virDomainOpenGraphics(virDomainPtr dom, unsigned int flags) { struct stat sb; + int rc; + VIR_DOMAIN_DEBUG(dom, "idx=%u, fd=%d, flags=0x%x", idx, fd, flags); @@ -10606,10 +10770,12 @@ virDomainOpenGraphics(virDomainPtr dom, virCheckReadOnlyGoto(dom->conn->flags, error); - if (!VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, - VIR_DRV_FEATURE_FD_PASSING)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("fd passing is not supported by this connection")); + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_FD_PASSING); + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("fd passing is not supported by this connection")); goto error; } @@ -10655,6 +10821,8 @@ virDomainOpenGraphicsFD(virDomainPtr dom, unsigned int idx, unsigned int flags) { + int rc; + VIR_DOMAIN_DEBUG(dom, "idx=%u, flags=0x%x", idx, flags); virResetLastError(); @@ -10663,10 +10831,13 @@ virDomainOpenGraphicsFD(virDomainPtr dom, virCheckReadOnlyGoto(dom->conn->flags, error); - if (!VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, - VIR_DRV_FEATURE_FD_PASSING)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("fd passing is not supported by this connection")); + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_FD_PASSING); + + if (rc <= 0) { + if (rc == 0) + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("fd passing is not supported by this connection")); goto error; } @@ -10789,6 +10960,7 @@ virDomainGetBlockIoTune(virDomainPtr dom, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(dom, "disk=%s, params=%p, nparams=%d, flags=0x%x", NULLSTR(disk), params, (nparams) ? *nparams : -1, flags); @@ -10804,8 +10976,11 @@ virDomainGetBlockIoTune(virDomainPtr dom, virCheckNonNullArgGoto(disk, error); } - if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, @@ -10915,6 +11090,7 @@ virDomainGetCPUStats(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, start_cpu=%d, ncpus=%u, flags=0x%x", @@ -10952,8 +11128,11 @@ virDomainGetCPUStats(virDomainPtr domain, nparams, ncpus); goto error; } - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->domainGetCPUStats) { @@ -12563,6 +12742,7 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, unsigned int flags) { virConnectPtr conn = domain->conn; + int rc; VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p flags=0x%x", params, nparams, flags); @@ -12574,8 +12754,11 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, virCheckNonNullArgGoto(nparams, error); virCheckReadOnlyGoto(conn->flags, error); - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->domainGetLaunchSecurityInfo) { diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 58622d4..8e680cb 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -772,6 +772,8 @@ virNodeGetMemoryParameters(virConnectPtr conn, int *nparams, unsigned int flags) { + int rc; + VIR_DEBUG("conn=%p, params=%p, nparams=%p, flags=0x%x", conn, params, nparams, flags); @@ -783,8 +785,11 @@ virNodeGetMemoryParameters(virConnectPtr conn, if (*nparams != 0) virCheckNonNullArgGoto(params, error); - if (VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->nodeGetMemoryParameters) { @@ -1724,6 +1729,8 @@ virNodeGetSEVInfo(virConnectPtr conn, int *nparams, unsigned int flags) { + int rc; + VIR_DEBUG("conn=%p, params=%p, nparams=%p, flags=0x%x", conn, params, nparams, flags); @@ -1734,8 +1741,11 @@ virNodeGetSEVInfo(virConnectPtr conn, virCheckNonNegativeArgGoto(*nparams, error); virCheckReadOnlyGoto(conn->flags, error); - if (VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, - VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + rc = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (rc < 0) + goto error; + if (rc) flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->nodeGetSEVInfo) { diff --git a/src/libvirt.c b/src/libvirt.c index 63c8bde..5778b5d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1336,12 +1336,13 @@ virTypedParameterValidateSet(virConnectPtr conn, virTypedParameterPtr params, int nparams) { - bool string_okay; + int string_okay; size_t i; - string_okay = VIR_DRV_SUPPORTS_FEATURE(conn->driver, - conn, + string_okay = VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING); + if (string_okay < 0) + return -1; for (i = 0; i < nparams; i++) { if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) == VIR_TYPED_PARAM_FIELD_LENGTH) { -- 1.8.3.1
On Fri, Dec 18, 2020 at 09:56:45AM +0300, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++++++----------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- 3 files changed, 365 insertions(+), 171 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/18/20 1:56 AM, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++++++----------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- 3 files changed, 365 insertions(+), 171 deletions(-)
[...]
@@ -3005,8 +3019,11 @@ virDomainMigrateVersion3Full(virDomainPtr domain, return NULL; params = tmp;
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) + ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (ret < 0) + return NULL; + if (ret)
Coverity complains this is a RESOURCE_LEAK for @tmp (or essentially @params) Perhaps the hunk for VIR_DRV_SUPPORTS_FEATURE should go before virTypedParamsCopy or use goto done (similar if !dom_xml)? John
protection = VIR_MIGRATE_CHANGE_PROTECTION;
VIR_DEBUG("Begin3 %p", domain->conn); @@ -3403,6 +3420,8 @@ virDomainMigrateUnmanagedParams(virDomainPtr domain,
[...]
On 1/7/21 12:53 PM, John Ferlan wrote:
On 12/18/20 1:56 AM, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++++++----------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- 3 files changed, 365 insertions(+), 171 deletions(-)
[...]
@@ -3005,8 +3019,11 @@ virDomainMigrateVersion3Full(virDomainPtr domain, return NULL; params = tmp;
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) + ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (ret < 0) + return NULL; + if (ret)
Coverity complains this is a RESOURCE_LEAK for @tmp (or essentially @params)
Perhaps the hunk for VIR_DRV_SUPPORTS_FEATURE should go before virTypedParamsCopy or use goto done (similar if !dom_xml)?
Yes, reorder looks good. Michal
Sigh.. One more issue with the patch series. I sent a tiny patch for formal approval. Nikolay On Thu, Jan 7, 2021 at 3:00 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/7/21 12:53 PM, John Ferlan wrote:
On 12/18/20 1:56 AM, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error
instead
of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 511 ++++++++++++++++++++++++++++++++++----------------- src/libvirt-host.c | 18 +- src/libvirt.c | 7 +- 3 files changed, 365 insertions(+), 171 deletions(-)
[...]
@@ -3005,8 +3019,11 @@ virDomainMigrateVersion3Full(virDomainPtr domain, return NULL; params = tmp;
- if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) + ret = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION); + if (ret < 0) + return NULL; + if (ret)
Coverity complains this is a RESOURCE_LEAK for @tmp (or essentially @params)
Perhaps the hunk for VIR_DRV_SUPPORTS_FEATURE should go before virTypedParamsCopy or use goto done (similar if !dom_xml)?
Yes, reorder looks good.
Michal
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_migration.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 6dc6812..2925aaa 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1146,7 +1146,7 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivatePtr driver, unsigned int flags) { int ret = -1; - bool useParams; + int useParams; virConnectPtr dconn = NULL; virErrorPtr orig_err = NULL; libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); @@ -1171,9 +1171,11 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivatePtr driver, VIR_DRV_FEATURE_MIGRATION_PARAMS); virObjectLock(vm); - if (!useParams) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Destination libvirt does not support migration with extensible parameters")); + if (useParams <= 0) { + if (useParams == 0) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Destination libvirt does not support migration" + " with extensible parameters")); goto cleanup; } -- 1.8.3.1
On Fri, Dec 18, 2020 at 09:56:46AM +0300, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_migration.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 90b0ec9..fca21be 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4706,12 +4706,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, { int ret = -1; g_autoptr(virConnect) dconn = NULL; - bool p2p; + int p2p; virErrorPtr orig_err = NULL; bool offline = !!(flags & VIR_MIGRATE_OFFLINE); - bool dstOffline = false; + int dstOffline; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - bool useParams; + int useParams; + int rc; VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, uri=%s, " "graphicsuri=%s, listenAddress=%s, nmigrate_disks=%zu, " @@ -4771,17 +4772,27 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, qemuDomainObjEnterRemote(vm); p2p = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_P2P); - /* v3proto reflects whether the caller used Perform3, but with - * p2p migrate, regardless of whether Perform2 or Perform3 - * were used, we decide protocol based on what target supports - */ - *v3proto = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, - VIR_DRV_FEATURE_MIGRATION_V3); + if (p2p < 0) + goto cleanup; + /* v3proto reflects whether the caller used Perform3, but with + * p2p migrate, regardless of whether Perform2 or Perform3 + * were used, we decide protocol based on what target supports + */ + rc = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, + VIR_DRV_FEATURE_MIGRATION_V3); + if (rc < 0) + goto cleanup; + *v3proto = !!rc; useParams = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_PARAMS); - if (offline) + if (useParams < 0) + goto cleanup; + if (offline) { dstOffline = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_OFFLINE); + if (dstOffline < 0) + goto cleanup; + } if (qemuDomainObjExitRemote(vm, !offline) < 0) goto cleanup; @@ -4819,7 +4830,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, persist_xml, dname, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, nbdPort, nbdURI, migParams, resource, - useParams, flags); + !!useParams, flags); } else { ret = qemuMigrationSrcPerformPeer2Peer2(driver, sconn, dconn, vm, dconnuri, flags, dname, resource, -- 1.8.3.1
On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 90b0ec9..fca21be 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4706,12 +4706,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, { int ret = -1; g_autoptr(virConnect) dconn = NULL; - bool p2p; + int p2p; virErrorPtr orig_err = NULL; bool offline = !!(flags & VIR_MIGRATE_OFFLINE); - bool dstOffline = false; + int dstOffline;
Loosing the initializer made the compiler unhappy it seems ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c: In function ‘qemuMigrationSrcPerformJob’: ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4814:20: error: ‘dstOffline’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 4814 | if (offline && !dstOffline) { | ^~~~~~~~~~~ ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4712:9: note: ‘dstOffline’ was declared here 4712 | int dstOffline; | ^~~~~~~~~~ Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Sorry, looks like I'm compiling with debug options and missed this error. It was a deliberate change but compiler don't like it. The build fix is pushed. Nikolay On Wed, Jan 6, 2021 at 5:44 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
Otherwise in some places we can mistakenly report 'unsupported' error instead of root cause. So let's handle root cause explicitly from the macro.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 90b0ec9..fca21be 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4706,12 +4706,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver, { int ret = -1; g_autoptr(virConnect) dconn = NULL; - bool p2p; + int p2p; virErrorPtr orig_err = NULL; bool offline = !!(flags & VIR_MIGRATE_OFFLINE); - bool dstOffline = false; + int dstOffline;
Loosing the initializer made the compiler unhappy it seems
../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c: In function ‘qemuMigrationSrcPerformJob’: ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4814:20: error: ‘dstOffline’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 4814 | if (offline && !dstOffline) { | ^~~~~~~~~~~ ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4712:9: note: ‘dstOffline’ was declared here 4712 | int dstOffline; | ^~~~~~~~~~
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Otherwise we can get misleading error messages. One example is when connection is broken we got "this function is not supported by the connection driver: virDomainMigrate3" from virDomainMigrate3. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/driver.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/driver.h b/src/driver.h index 6278aa0..2531ac3 100644 --- a/src/driver.h +++ b/src/driver.h @@ -47,17 +47,14 @@ typedef enum { * directly if you don't have to, because it may be NULL, use this macro * instead. * - * Note that this treats a possible error returned by drv->supports_feature - * the same as not supported. If you care about the error, call - * drv->supports_feature directly. - * * Returns: - * != 0 Feature is supported. + * -1 Error + * >0 Feature is supported. * 0 Feature is not supported. */ #define VIR_DRV_SUPPORTS_FEATURE(drv, conn, feature) \ ((drv)->connectSupportsFeature ? \ - (drv)->connectSupportsFeature((conn), (feature)) > 0 : 0) + (drv)->connectSupportsFeature((conn), (feature)) : 0) #define __VIR_DRIVER_H_INCLUDES___ -- 1.8.3.1
On Fri, Dec 18, 2020 at 09:56:48AM +0300, Nikolay Shirokovskiy wrote:
Otherwise we can get misleading error messages. One example is when connection is broken we got "this function is not supported by the connection driver: virDomainMigrate3" from virDomainMigrate3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/driver.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik -
Nikolay Shirokovskiy