12:56 a.m.
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
12:56 a.m.
New subject: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)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
6:33 a.m.
New subject: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)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(a)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 :|
5:53 a.m.
New subject: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
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(a)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,
[...]
6 a.m.
New subject: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
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(a)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
7:58 a.m.
New subject: [PATCH v3 1/4] src: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
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(a)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(a)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
12:56 a.m.
New subject: [PATCH v3 2/4] libxl: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)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
6:33 a.m.
New subject: [PATCH v3 2/4] libxl: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)virtuozzo.com>
---
src/libxl/libxl_migration.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
12:56 a.m.
New subject: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)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
6:34 a.m.
New subject: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)virtuozzo.com>
---
src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
8:44 a.m.
New subject: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -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(a)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 :|
9:49 a.m.
New subject: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
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(a)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(a)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 :|
12:56 a.m.
New subject: [PATCH v3 4/4] src: don't hide error in VIR_DRV_SUPPORTS_FEATURE
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(a)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
6:35 a.m.
New subject: [PATCH v3 4/4] src: don't hide error in VIR_DRV_SUPPORTS_FEATURE
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(a)virtuozzo.com>
---
src/driver.h | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
1414
days inactive
1434
days old
13 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik -
Nikolay Shirokovskiy