3:43 p.m.
v2 of my patch series started here, after applying all patches
already reviewed in that thread:
https://www.redhat.com/archives/libvir-list/2013-December/msg01284.html
Patches 1-3 can be considered bug fixes (particularly patch 3),
so I'd like them in 1.2.1 if they get a favorable review. Patches
4-6 are less urgent, but might as well finish the work all within
one release.
Patch 1 comes from 5/24 in v1
Patch 2 is new
Patch 3 and 4 come from 23/24 in v1
Patch 5 is new
Patch 6 comes from 24/24 in v1
Eric Blake (6):
maint: don't leave garbage on early API exit
maint: avoid nested use of virConnect{Ref,Close}
maint: don't lose error on canceled migration
maint: clean up error reporting in migration
maint: simplify driver registration at startup
maint: replace remaining virLib*Error with better names
cfg.mk | 10 --
src/libvirt.c | 397 ++++++++++++++++++++-----------------------
src/lxc/lxc_process.c | 11 +-
src/qemu/qemu_driver.c | 12 +-
src/qemu/qemu_migration.c | 14 +-
src/qemu/qemu_process.c | 10 +-
src/storage/storage_driver.c | 5 +-
src/uml/uml_driver.c | 3 +-
8 files changed, 209 insertions(+), 253 deletions(-)
--
1.8.4.2
3:43 p.m.
New subject: [libvirt] [PATCHv2 1/6] maint: don't leave garbage on early API exit
Several APIs clear out a user input buffer before attempting to
populate it; but in a few cases we missed this memset if we
detect a reason for an early exit. Note that these APIs
check for non-NULL arguments, and exit early with an error
message when NULL is passed in; which means that we must be
careful to avoid a NULL deref in order to get to that error
message. Also, we were inconsistent on the use of
sizeof(virType) vs. sizeof(expression); the latter is more
robust if we ever change the type of the expression (although
such action is unlikely since these types are part of our
public API).
* src/libvirt.c (virDomainGetInfo, virDomainGetBlockInfo)
(virStoragePoolGetInfo, virStorageVolGetInfo)
(virDomainGetJobInfo, virDomainGetBlockJobInfo): Move memset
before any returns.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2 avoid null deref, prefer sizeof(expr)
src/libvirt.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 87a4d46..6357c49 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4144,11 +4144,12 @@ virDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
virResetLastError();
+ if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckDomainReturn(domain, -1);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virDomainInfo));
-
conn = domain->conn;
if (conn->driver->domainGetInfo) {
@@ -8449,12 +8450,12 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *disk,
virResetLastError();
+ memset(info, 0, sizeof(*info));
+
virCheckDomainReturn(domain, -1);
virCheckNonNullArgGoto(disk, error);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virDomainBlockInfo));
-
conn = domain->conn;
if (conn->driver->domainGetBlockInfo) {
@@ -13082,11 +13083,12 @@ virStoragePoolGetInfo(virStoragePoolPtr pool,
virResetLastError();
+ if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckStoragePoolReturn(pool, -1);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virStoragePoolInfo));
-
conn = pool->conn;
if (conn->storageDriver->storagePoolGetInfo) {
@@ -13951,11 +13953,12 @@ virStorageVolGetInfo(virStorageVolPtr vol,
virResetLastError();
+ if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckStorageVolReturn(vol, -1);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virStorageVolInfo));
-
conn = vol->conn;
if (conn->storageDriver->storageVolGetInfo){
@@ -17240,11 +17243,12 @@ virDomainGetJobInfo(virDomainPtr domain, virDomainJobInfoPtr
info)
virResetLastError();
+ if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckDomainReturn(domain, -1);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virDomainJobInfo));
-
conn = domain->conn;
if (conn->driver->domainGetJobInfo) {
@@ -19379,14 +19383,15 @@ virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk,
virResetLastError();
+ if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckDomainReturn(dom, -1);
conn = dom->conn;
virCheckNonNullArgGoto(disk, error);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(*info));
-
if (conn->driver->domainGetBlockJobInfo) {
int ret;
ret = conn->driver->domainGetBlockJobInfo(dom, disk, info, flags);
--
1.8.4.2
8:36 a.m.
New subject: [libvirt] [PATCHv2 1/6] maint: don't leave garbage on early API exit
On 01/14/2014 10:43 PM, Eric Blake wrote:
Several APIs clear out a user input buffer before attempting to
populate it; but in a few cases we missed this memset if we
detect a reason for an early exit. Note that these APIs
check for non-NULL arguments, and exit early with an error
message when NULL is passed in; which means that we must be
careful to avoid a NULL deref in order to get to that error
message. Also, we were inconsistent on the use of
sizeof(virType) vs. sizeof(expression); the latter is more
robust if we ever change the type of the expression (although
such action is unlikely since these types are part of our
public API).
* src/libvirt.c (virDomainGetInfo, virDomainGetBlockInfo)
(virStoragePoolGetInfo, virStorageVolGetInfo)
(virDomainGetJobInfo, virDomainGetBlockJobInfo): Move memset
before any returns.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2 avoid null deref, prefer sizeof(expr)
src/libvirt.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
@@ -8449,12 +8450,12 @@ virDomainGetBlockInfo(virDomainPtr domain,
const char *disk,
virResetLastError();
if (info)
+ memset(info, 0, sizeof(*info));
+
virCheckDomainReturn(domain, -1);
virCheckNonNullArgGoto(disk, error);
virCheckNonNullArgGoto(info, error);
- memset(info, 0, sizeof(virDomainBlockInfo));
-
conn = domain->conn;
if (conn->driver->domainGetBlockInfo) {
Jan
3:43 p.m.
New subject: [libvirt] [PATCHv2 2/6] maint: avoid nested use of virConnect{Ref, Close}
The public virConnectRef and virConnectClose API are just thin
wrappers around virObjectRef/virObjectRef, with added object
validation and an error reset. Within our backend drivers, use
of the object validation is just an inefficiency since we always
pass valid objects. More important to think about is what
happens with the error reset; our uses of virConnectRef happened
to be safe (since we hadn't encountered any earlier errors), but
in several cases the use of virConnectClose could lose a real
error.
Ideally, we should also avoid calling virConnectOpen() from
within backend drivers - but that is a known situation that
needs much more design work.
* src/qemu/qemu_process.c (qemuProcessReconnectHelper)
(qemuProcessReconnect): Avoid nested public API call.
* src/qemu/qemu_driver.c (qemuAutostartDomains)
(qemuStateInitialize, qemuStateStop): Likewise.
* src/qemu/qemu_migration.c (doPeer2PeerMigrate): Likewise.
* src/storage/storage_driver.c (storageDriverAutostart):
Likewise.
* src/uml/uml_driver.c (umlAutostartConfigs): Likewise.
* src/lxc/lxc_process.c (virLXCProcessAutostartAll): Likewise.
(virLXCProcessReboot): Likewise, and avoid leaking conn on error.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/lxc/lxc_process.c | 11 ++++-------
src/qemu/qemu_driver.c | 12 ++++--------
src/qemu/qemu_migration.c | 4 ++--
src/qemu/qemu_process.c | 10 +++++-----
src/storage/storage_driver.c | 5 ++---
src/uml/uml_driver.c | 3 +--
6 files changed, 18 insertions(+), 27 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index cc9c1a2..ed729f6 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2010-2013 Red Hat, Inc.
+ * Copyright (C) 2010-2014 Red Hat, Inc.
* Copyright IBM Corp. 2008
*
* lxc_process.c: LXC process lifecycle management
@@ -103,7 +103,7 @@ virLXCProcessReboot(virLXCDriverPtr driver,
VIR_DEBUG("Faking reboot");
if (conn) {
- virConnectRef(conn);
+ virObjectRef(conn);
autodestroy = true;
} else {
conn = virConnectOpen("lxc:///");
@@ -125,12 +125,10 @@ virLXCProcessReboot(virLXCDriverPtr driver,
goto cleanup;
}
- if (conn)
- virConnectClose(conn);
-
ret = 0;
cleanup:
+ virObjectUnref(conn);
return ret;
}
@@ -1428,8 +1426,7 @@ virLXCProcessAutostartAll(virLXCDriverPtr driver)
virLXCProcessAutostartDomain,
&data);
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
}
static int
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1949abe..6389a8b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -320,8 +320,7 @@ qemuAutostartDomains(virQEMUDriverPtr driver)
virDomainObjListForEach(driver->domains, qemuAutostartDomain, &data);
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
virObjectUnref(cfg);
}
@@ -843,15 +842,13 @@ qemuStateInitialize(bool privileged,
if (!qemu_driver->workerPool)
goto error;
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
virNWFilterRegisterCallbackDriver(&qemuCallbackDriver);
return 0;
error:
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
VIR_FREE(driverConf);
VIR_FREE(membase);
VIR_FREE(mempath);
@@ -970,8 +967,7 @@ qemuStateStop(void) {
virDomainFree(domains[i]);
VIR_FREE(domains);
VIR_FREE(flags);
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
virObjectUnref(cfg);
return ret;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 53a4ae6..a8a493a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2,7 +2,7 @@
/*
* qemu_migration.c: QEMU migration handling
*
- * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006-2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -4021,7 +4021,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver,
cleanup:
orig_err = virSaveLastError();
qemuDomainObjEnterRemote(vm);
- virConnectClose(dconn);
+ virObjectUnref(dconn);
qemuDomainObjExitRemote(vm);
if (orig_err) {
virSetError(orig_err);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9331744..28aa45c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1,7 +1,7 @@
/*
* qemu_process.c: QEMU process management
*
- * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006-2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -3261,7 +3261,7 @@ endjob:
if (obj && virObjectUnref(obj))
virObjectUnlock(obj);
- virConnectClose(conn);
+ virObjectUnref(conn);
virObjectUnref(cfg);
return;
@@ -3296,7 +3296,7 @@ error:
virObjectUnlock(obj);
}
}
- virConnectClose(conn);
+ virObjectUnref(conn);
virObjectUnref(cfg);
}
@@ -3343,11 +3343,11 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
* that the threads we start see a valid connection throughout their
* lifetime. We simply increase the reference counter here.
*/
- virConnectRef(data->conn);
+ virObjectRef(data->conn);
if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) {
- virConnectClose(data->conn);
+ virObjectUnref(data->conn);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Could not create thread. QEMU initialization "
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 85fc0f2..36eb793 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1,7 +1,7 @@
/*
* storage_driver.c: core driver for storage APIs
*
- * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006-2014 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -130,8 +130,7 @@ storageDriverAutostart(virStorageDriverStatePtr driver) {
virStoragePoolObjUnlock(pool);
}
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
}
/**
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index ad29ebf..6a1b129 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -224,8 +224,7 @@ umlAutostartConfigs(struct uml_driver *driver) {
virDomainObjListForEach(driver->domains, umlAutostartDomain, &data);
umlDriverUnlock(driver);
- if (conn)
- virConnectClose(conn);
+ virObjectUnref(conn);
}
--
1.8.4.2
3:43 p.m.
New subject: [libvirt] [PATCHv2 3/6] maint: don't lose error on canceled migration
While auditing the error reporting, I noticed that migration
had some issues. Some of the static helper functions tried
to call virDispatchError(), even though their caller will also
report the error. Also, if a migration is cancelled early
because a uri was not set, we did not guarantee that the finish
stage would not overwrite the first error message.
* src/qemu/qemu_migration.c (doPeer2PeerMigrate2)
(doPeer2PeerMigrate3): Preserve first error when cancelling.
* src/libvirt.c (virDomainMigrateVersion3Full): Likewise.
(virDomainMigrateVersion1, virDomainMigrateVersion2)
(virDomainMigrateDirect): Avoid redundant error dispatch.
(virDomainMigrateFinish2, virDomainMigrateFinish3)
(virDomainMigrateFinish3Params): Don't report error on cleanup
path.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2 split bug reporting changes out from error message changes
src/libvirt.c | 23 ++++++++++++++++-------
src/qemu/qemu_migration.c | 10 +++++++++-
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 6357c49..fce0ec4 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4561,7 +4561,6 @@ virDomainMigrateVersion2(virDomainPtr domain,
*/
if (!domain->conn->driver->domainGetXMLDesc) {
virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
- virDispatchError(domain->conn);
return NULL;
}
@@ -4593,8 +4592,9 @@ virDomainMigrateVersion2(virDomainPtr domain,
if (uri == NULL && uri_out == NULL) {
virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare2 did not set uri"));
- virDispatchError(domain->conn);
cancelled = 1;
+ /* Make sure Finish doesn't overwrite the error */
+ orig_err = virSaveLastError();
goto finish;
}
if (uri_out)
@@ -4625,6 +4625,8 @@ finish:
VIR_DEBUG("Finish2 %p ret=%d", dconn, ret);
ddomain = dconn->driver->domainMigrateFinish2
(dconn, dname, cookie, cookielen, uri, destflags, cancelled);
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
done:
if (orig_err) {
@@ -4787,13 +4789,19 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
if (useParams &&
virTypedParamsReplaceString(¶ms, &nparams,
VIR_MIGRATE_PARAM_URI,
- uri_out) < 0)
+ uri_out) < 0) {
+ cancelled = 1;
+ orig_err = virSaveLastError();
goto finish;
+ }
} else if (!uri &&
virTypedParamsGetString(params, nparams,
VIR_MIGRATE_PARAM_URI, &uri) <= 0) {
virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare3 did not set uri"));
+ cancelled = 1;
+ orig_err = virSaveLastError();
+ goto finish;
}
if (flags & VIR_MIGRATE_OFFLINE) {
@@ -4872,6 +4880,8 @@ finish:
(dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
NULL, uri, destflags, cancelled);
}
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
@@ -5097,7 +5107,6 @@ virDomainMigrateDirect(virDomainPtr domain,
if (!domain->conn->driver->domainMigratePerform) {
virReportUnsupportedError();
- virDispatchError(domain->conn);
return -1;
}
@@ -6403,7 +6412,7 @@ virDomainMigrateFinish2(virConnectPtr dconn,
cookie, cookielen,
uri, flags,
retcode);
- if (!ret)
+ if (!ret && !retcode)
goto error;
return ret;
}
@@ -6695,7 +6704,7 @@ virDomainMigrateFinish3(virConnectPtr dconn,
cookieout, cookieoutlen,
dconnuri, uri, flags,
cancelled);
- if (!ret)
+ if (!ret && !cancelled)
goto error;
return ret;
}
@@ -6970,7 +6979,7 @@ virDomainMigrateFinish3Params(virConnectPtr dconn,
ret = dconn->driver->domainMigrateFinish3Params(
dconn, params, nparams, cookiein, cookieinlen,
cookieout, cookieoutlen, flags, cancelled);
- if (!ret)
+ if (!ret && !cancelled)
goto error;
return ret;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a8a493a..407fb70 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3569,6 +3569,7 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare2 did not set uri"));
cancelled = true;
+ orig_err = virSaveLastError();
goto finish;
}
@@ -3608,6 +3609,8 @@ finish:
(dconn, dname, cookie, cookielen,
uri_out ? uri_out : dconnuri, destflags, cancelled);
qemuDomainObjExitRemote(vm);
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
cleanup:
if (ddomain) {
@@ -3769,11 +3772,14 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
uri = uri_out;
if (useParams &&
virTypedParamsReplaceString(¶ms, &nparams,
- VIR_MIGRATE_PARAM_URI, uri_out) < 0)
+ VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
+ orig_err = virSaveLastError();
goto finish;
+ }
} else if (!uri && !(flags & VIR_MIGRATE_TUNNELLED)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare3 did not set uri"));
+ orig_err = virSaveLastError();
goto finish;
}
@@ -3850,6 +3856,8 @@ finish:
dconnuri, uri, destflags, cancelled);
qemuDomainObjExitRemote(vm);
}
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
--
1.8.4.2
3:43 p.m.
New subject: [libvirt] [PATCHv2 4/6] maint: clean up error reporting in migration
The choice of error message and category was not consistent
in the migration code; furthermore, the use of virLibConnError
is no longer necessary now that we have a generic virReportError.
* src/qemu/qemu_migration.c (virDomainMigrate*): Prefer
virReportError over virLibConnError.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/libvirt.c | 158 +++++++++++++++++++++++++++++-----------------------------
1 file changed, 79 insertions(+), 79 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index fce0ec4..2643f2d 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4468,8 +4468,8 @@ virDomainMigrateVersion1(virDomainPtr domain,
goto done;
if (uri == NULL && uri_out == NULL) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare did not set uri"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare did not set uri"));
goto done;
}
if (uri_out)
@@ -4560,7 +4560,7 @@ virDomainMigrateVersion2(virDomainPtr domain,
* and pass it to Prepare2.
*/
if (!domain->conn->driver->domainGetXMLDesc) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ virReportUnsupportedError();
return NULL;
}
@@ -4590,8 +4590,8 @@ virDomainMigrateVersion2(virDomainPtr domain,
goto done;
if (uri == NULL && uri_out == NULL) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare2 did not set uri"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare2 did not set uri"));
cancelled = 1;
/* Make sure Finish doesn't overwrite the error */
orig_err = virSaveLastError();
@@ -4715,7 +4715,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
!domain->conn->driver->domainMigrateConfirm3Params ||
!dconn->driver->domainMigratePrepare3Params ||
!dconn->driver->domainMigrateFinish3Params))) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ virReportUnsupportedError();
return NULL;
}
@@ -4797,8 +4797,8 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
} else if (!uri &&
virTypedParamsGetString(params, nparams,
VIR_MIGRATE_PARAM_URI, &uri) <= 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare3 did not set uri"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare3 did not set uri"));
cancelled = 1;
orig_err = virSaveLastError();
goto finish;
@@ -5010,7 +5010,7 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain,
(!useParams &&
!domain->conn->driver->domainMigratePerform &&
!domain->conn->driver->domainMigratePerform3)) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ virReportUnsupportedError();
return -1;
}
@@ -5039,14 +5039,14 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain,
} else {
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML "
- "during migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
return -1;
}
if (uri) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to override peer2peer migration URI"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to override peer2peer migration URI"));
return -1;
}
return domain->conn->driver->domainMigratePerform
@@ -5132,8 +5132,8 @@ virDomainMigrateDirect(virDomainPtr domain,
} else {
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during
migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during
migration"));
return -1;
}
return domain->conn->driver->domainMigratePerform(domain,
@@ -5261,16 +5261,16 @@ virDomainMigrate(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ 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)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5308,14 +5308,14 @@ virDomainMigrate(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
if (flags & VIR_MIGRATE_TUNNELLED) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot perform tunnelled migration without using
peer2peer flag"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot perform tunnelled migration without using
peer2peer flag"));
goto error;
}
@@ -5487,16 +5487,16 @@ virDomainMigrate2(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ 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)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5531,14 +5531,14 @@ virDomainMigrate2(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
if (flags & VIR_MIGRATE_TUNNELLED) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot perform tunnelled migration without using
peer2peer flag"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot perform tunnelled migration without using
peer2peer flag"));
goto error;
}
@@ -5556,8 +5556,8 @@ virDomainMigrate2(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V2)) {
VIR_DEBUG("Using migration protocol 2");
if (dxml) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during
migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during
migration"));
goto error;
}
ddomain = virDomainMigrateVersion2(domain, dconn, flags,
@@ -5568,8 +5568,8 @@ virDomainMigrate2(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V1)) {
VIR_DEBUG("Using migration protocol 1");
if (dxml) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during
migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during
migration"));
goto error;
}
ddomain = virDomainMigrateVersion1(domain, dconn, flags,
@@ -5670,16 +5670,16 @@ virDomainMigrate3(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ 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)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5692,8 +5692,8 @@ virDomainMigrate3(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
@@ -5712,9 +5712,9 @@ virDomainMigrate3(virDomainPtr domain,
if (!virTypedParamsCheck(params, nparams, compatParams,
ARRAY_CARDINALITY(compatParams))) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Migration APIs with extensible parameters are not "
- "supported but extended parameters were passed"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Migration APIs with extensible parameters are not "
+ "supported but extended parameters were passed"));
goto error;
}
@@ -5742,9 +5742,9 @@ virDomainMigrate3(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V2)) {
VIR_DEBUG("Using migration protocol 2");
if (dxml) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML during "
- "migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
goto error;
}
ddomain = virDomainMigrateVersion2(domain, dconn, flags,
@@ -5755,9 +5755,9 @@ virDomainMigrate3(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V1)) {
VIR_DEBUG("Using migration protocol 1");
if (dxml) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML during "
- "migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
goto error;
}
ddomain = virDomainMigrateVersion1(domain, dconn, flags,
@@ -5881,9 +5881,9 @@ virDomainMigrateToURI(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
goto error;
}
@@ -5908,9 +5908,9 @@ virDomainMigrateToURI(virDomainPtr domain,
goto error;
} else {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("direct migration is not supported by the"
+ " connection driver"));
goto error;
}
}
@@ -6056,9 +6056,9 @@ virDomainMigrateToURI2(virDomainPtr domain,
goto error;
} else {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("direct migration is not supported by the"
+ " connection driver"));
goto error;
}
}
@@ -6161,9 +6161,9 @@ virDomainMigrateToURI3(virDomainPtr domain,
if (flags & VIR_MIGRATE_PEER2PEER) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_P2P)) {
- virLibConnError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("Peer-to-peer migration is not supported by "
- "the connection driver"));
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Peer-to-peer migration is not supported by "
+ "the connection driver"));
goto error;
}
@@ -6179,26 +6179,26 @@ virDomainMigrateToURI3(virDomainPtr domain,
dconnuri, uri, bandwidth) < 0)
goto error;
} else {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Peer-to-peer migration with extensible "
- "parameters is not supported but extended "
- "parameters were passed"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Peer-to-peer migration with extensible "
+ "parameters is not supported but extended "
+ "parameters were passed"));
goto error;
}
} else {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("Direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Direct migration is not supported by the"
+ " connection driver"));
goto error;
}
if (!compat) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Direct migration does not support extensible "
- "parameters"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Direct migration does not support extensible "
+ "parameters"));
goto error;
}
--
1.8.4.2
3:43 p.m.
New subject: [libvirt] [PATCHv2 5/6] maint: simplify driver registration at startup
We had a lot of repetition of errors that would occur if we
ever register too many drivers; this is unlikely to occur
unless we start adding a lot of new hypervisor modules, but
if it does occur, it's better to have uniform handling of the
situation, so that a one-line change is all that would be
needed if we decide that an internal error is not the best.
* src/libvirt.c (virDriverCheckTabMaxReturn): New define.
(virRegister*Driver): Use it for less code duplication.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2: new patch, suggested by John Ferlan
src/libvirt.c | 77 ++++++++++++++++-------------------------------------------
1 file changed, 20 insertions(+), 57 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 2643f2d..b401f18 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -107,6 +107,16 @@
#define MAX_DRIVERS 20
+#define virDriverCheckTabMaxReturn(count, ret) \
+ do { \
+ if ((count) >= MAX_DRIVERS) { \
+ virReportError(VIR_ERR_INTERNAL_ERROR, \
+ _("Too many drivers, cannot register %s"), \
+ driver->name); \
+ return ret; \
+ } \
+ } while (0)
+
static virDriverPtr virDriverTab[MAX_DRIVERS];
static int virDriverTabCount = 0;
static virNetworkDriverPtr virNetworkDriverTab[MAX_DRIVERS];
@@ -542,13 +552,7 @@ int
virRegisterNetworkDriver(virNetworkDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virNetworkDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virNetworkDriverTabCount, -1);
VIR_DEBUG("registering %s as network driver %d",
driver->name, virNetworkDriverTabCount);
@@ -570,13 +574,7 @@ int
virRegisterInterfaceDriver(virInterfaceDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virInterfaceDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virInterfaceDriverTabCount, -1);
VIR_DEBUG("registering %s as interface driver %d",
driver->name, virInterfaceDriverTabCount);
@@ -598,13 +596,7 @@ int
virRegisterStorageDriver(virStorageDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virStorageDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virStorageDriverTabCount, -1);
VIR_DEBUG("registering %s as storage driver %d",
driver->name, virStorageDriverTabCount);
@@ -626,13 +618,7 @@ int
virRegisterNodeDeviceDriver(virNodeDeviceDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virNodeDeviceDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virNodeDeviceDriverTabCount, -1);
VIR_DEBUG("registering %s as device driver %d",
driver->name, virNodeDeviceDriverTabCount);
@@ -654,13 +640,7 @@ int
virRegisterSecretDriver(virSecretDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virSecretDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virSecretDriverTabCount, -1);
VIR_DEBUG("registering %s as secret driver %d",
driver->name, virSecretDriverTabCount);
@@ -682,13 +662,7 @@ int
virRegisterNWFilterDriver(virNWFilterDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virNWFilterDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virNWFilterDriverTabCount, -1);
VIR_DEBUG("registering %s as network filter driver %d",
driver->name, virNWFilterDriverTabCount);
@@ -709,16 +683,11 @@ virRegisterNWFilterDriver(virNWFilterDriverPtr driver)
int
virRegisterDriver(virDriverPtr driver)
{
- VIR_DEBUG("driver=%p name=%s", driver, driver ? NULLSTR(driver->name) :
"(null)");
+ VIR_DEBUG("driver=%p name=%s", driver,
+ driver ? NULLSTR(driver->name) : "(null)");
virCheckNonNullArgReturn(driver, -1);
-
- if (virDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virDriverTabCount, -1);
VIR_DEBUG("registering %s as driver %d",
driver->name, virDriverTabCount);
@@ -741,13 +710,7 @@ int
virRegisterStateDriver(virStateDriverPtr driver)
{
virCheckNonNullArgReturn(driver, -1);
-
- if (virStateDriverTabCount >= MAX_DRIVERS) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR,
- _("Too many drivers, cannot register %s"),
- driver->name);
- return -1;
- }
+ virDriverCheckTabMaxReturn(virStateDriverTabCount, -1);
virStateDriverTab[virStateDriverTabCount] = driver;
return virStateDriverTabCount++;
--
1.8.4.2
3:43 p.m.
New subject: [libvirt] [PATCHv2 6/6] maint: replace remaining virLib*Error with better names
Finish the cleanup of libvirt.c; all uses of virLib*Error have
now been converted to more canonical conventions.
* src/libvirt.c: Use virReportError in remaining errors.
(virLibConnError, virLibDomainError): Delete unused macros.
* cfg.mk (msg_gen_function): Drop unused names.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
cfg.mk | 10 ------
src/libvirt.c | 110 ++++++++++++++++++++++++++--------------------------------
2 files changed, 50 insertions(+), 70 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 5591065..e7515ea 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -585,16 +585,6 @@ msg_gen_function += regerror
msg_gen_function += vah_error
msg_gen_function += vah_warning
msg_gen_function += virGenericReportError
-msg_gen_function += virLibConnError
-msg_gen_function += virLibDomainError
-msg_gen_function += virLibDomainSnapshotError
-msg_gen_function += virLibInterfaceError
-msg_gen_function += virLibNetworkError
-msg_gen_function += virLibNodeDeviceError
-msg_gen_function += virLibNWFilterError
-msg_gen_function += virLibSecretError
-msg_gen_function += virLibStoragePoolError
-msg_gen_function += virLibStorageVolError
msg_gen_function += virRaiseError
msg_gen_function += virReportError
msg_gen_function += virReportErrorHelper
diff --git a/src/libvirt.c b/src/libvirt.c
index b401f18..5848cb5 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -532,14 +532,6 @@ DllMain(HINSTANCE instance ATTRIBUTE_UNUSED,
#endif
-#define virLibConnError(code, ...) \
- virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \
- __FUNCTION__, __LINE__, __VA_ARGS__)
-#define virLibDomainError(code, ...) \
- virReportErrorHelper(VIR_FROM_DOM, code, __FILE__, \
- __FUNCTION__, __LINE__, __VA_ARGS__)
-
-
/**
* virRegisterNetworkDriver:
* @driver: pointer to a network driver block
@@ -944,8 +936,8 @@ virConnectOpenFindURIAliasMatch(virConfValuePtr value, const char
*alias,
size_t alias_len;
if (value->type != VIR_CONF_LIST) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Expected a list for 'uri_aliases' config
parameter"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Expected a list for 'uri_aliases' config
parameter"));
return -1;
}
@@ -956,22 +948,22 @@ virConnectOpenFindURIAliasMatch(virConfValuePtr value, const char
*alias,
size_t safe;
if (entry->type != VIR_CONF_STRING) {
- virLibConnError(VIR_ERR_CONF_SYNTAX, "%s",
- _("Expected a string for 'uri_aliases' config
parameter list entry"));
+ virReportError(VIR_ERR_CONF_SYNTAX, "%s",
+ _("Expected a string for 'uri_aliases' config
parameter list entry"));
return -1;
}
if (!(offset = strchr(entry->str, '='))) {
- virLibConnError(VIR_ERR_CONF_SYNTAX,
- _("Malformed 'uri_aliases' config entry
'%s', expected 'alias=uri://host/path'"),
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("Malformed 'uri_aliases' config entry
'%s', expected 'alias=uri://host/path'"),
entry->str);
return -1;
}
safe = strspn(entry->str, URI_ALIAS_CHARS);
if (safe < (offset - entry->str)) {
- virLibConnError(VIR_ERR_CONF_SYNTAX,
- _("Malformed 'uri_aliases' config entry
'%s', aliases may only contain 'a-Z, 0-9, _, -'"),
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("Malformed 'uri_aliases' config entry
'%s', aliases may only contain 'a-Z, 0-9, _, -'"),
entry->str);
return -1;
}
@@ -1022,8 +1014,8 @@ virConnectGetDefaultURI(virConfPtr conf,
*name = defname;
} else if ((value = virConfGetValue(conf, "uri_default"))) {
if (value->type != VIR_CONF_STRING) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Expected a string for 'uri_default' config
parameter"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Expected a string for 'uri_default' config
parameter"));
goto cleanup;
}
VIR_DEBUG("Using config file uri '%s'", value->str);
@@ -1168,9 +1160,7 @@ do_open(const char *name,
if (!ret->driver) {
/* If we reach here, then all drivers declined the connection. */
- virLibConnError(VIR_ERR_NO_CONNECT,
- "%s",
- NULLSTR(name));
+ virReportError(VIR_ERR_NO_CONNECT, "%s", NULLSTR(name));
goto failed;
}
@@ -2539,8 +2529,8 @@ virDomainSave(virDomainPtr domain, const char *to)
/* We must absolutize the file path as the save is done out of process */
if (virFileAbsPath(to, &absolute_to) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute output file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute output file path"));
goto error;
}
@@ -2630,8 +2620,8 @@ virDomainSaveFlags(virDomainPtr domain, const char *to,
/* We must absolutize the file path as the save is done out of process */
if (virFileAbsPath(to, &absolute_to) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute output file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute output file path"));
goto error;
}
@@ -2680,8 +2670,8 @@ virDomainRestore(virConnectPtr conn, const char *from)
/* We must absolutize the file path as the restore is done out of process */
if (virFileAbsPath(from, &absolute_from) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute input file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute input file path"));
goto error;
}
@@ -2757,8 +2747,8 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const
char *dxml,
/* We must absolutize the file path as the restore is done out of process */
if (virFileAbsPath(from, &absolute_from) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute input file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute input file path"));
goto error;
}
@@ -2811,8 +2801,8 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file,
virCheckNonNullArgGoto(file, error);
if ((conn->flags & VIR_CONNECT_RO) && (flags &
VIR_DOMAIN_XML_SECURE)) {
- virLibConnError(VIR_ERR_OPERATION_DENIED, "%s",
- _("virDomainSaveImageGetXMLDesc with secure flag"));
+ virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("virDomainSaveImageGetXMLDesc with secure flag"));
goto error;
}
@@ -2822,8 +2812,8 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file,
/* We must absolutize the file path as the read is done out of process */
if (virFileAbsPath(file, &absolute_file) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute input file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute input file path"));
goto error;
}
@@ -2898,8 +2888,8 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file,
/* We must absolutize the file path as the read is done out of process */
if (virFileAbsPath(file, &absolute_file) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute input file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute input file path"));
goto error;
}
@@ -2985,8 +2975,8 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int
flags)
/* We must absolutize the file path as the save is done out of process */
if (virFileAbsPath(to, &absolute_to) < 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not build absolute core file path"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not build absolute core file path"));
goto error;
}
@@ -3445,8 +3435,8 @@ virDomainGetMaxMemory(virDomainPtr domain)
if (ret == 0)
goto error;
if ((unsigned long) ret != ret) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("result too large: %llu"),
- ret);
+ virReportError(VIR_ERR_OVERFLOW, _("result too large: %llu"),
+ ret);
goto error;
}
return ret;
@@ -4252,8 +4242,8 @@ virDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
conn = domain->conn;
if ((conn->flags & VIR_CONNECT_RO) && (flags &
VIR_DOMAIN_XML_SECURE)) {
- virLibConnError(VIR_ERR_OPERATION_DENIED, "%s",
- _("virDomainGetXMLDesc with secure flag"));
+ virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("virDomainGetXMLDesc with secure flag"));
goto error;
}
@@ -9312,7 +9302,7 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus,
virCheckNonZeroArgGoto(nvcpus, error);
if ((unsigned short) nvcpus != nvcpus) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), nvcpus);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), nvcpus);
goto error;
}
conn = domain->conn;
@@ -9439,7 +9429,7 @@ virDomainPinVcpu(virDomainPtr domain, unsigned int vcpu,
virCheckPositiveArgGoto(maplen, error);
if ((unsigned short) vcpu != vcpu) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu);
goto error;
}
@@ -9511,7 +9501,7 @@ virDomainPinVcpuFlags(virDomainPtr domain, unsigned int vcpu,
virCheckPositiveArgGoto(maplen, error);
if ((unsigned short) vcpu != vcpu) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu);
goto error;
}
@@ -9574,8 +9564,8 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int ncpumaps,
virCheckPositiveArgGoto(maplen, error);
if (INT_MULTIPLY_OVERFLOW(ncpumaps, maplen)) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"),
- ncpumaps, maplen);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"),
+ ncpumaps, maplen);
goto error;
}
@@ -9786,8 +9776,8 @@ virDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int
maxinfo,
virCheckZeroArgGoto(maplen, error);
if (cpumaps && INT_MULTIPLY_OVERFLOW(maxinfo, maplen)) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"),
- maxinfo, maplen);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"),
+ maxinfo, maplen);
goto error;
}
@@ -15888,8 +15878,8 @@ virStreamSendAll(virStreamPtr stream,
virCheckNonNullArgGoto(handler, cleanup);
if (stream->flags & VIR_STREAM_NONBLOCK) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("data sources cannot be used for non-blocking
streams"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("data sources cannot be used for non-blocking
streams"));
goto cleanup;
}
@@ -15981,8 +15971,8 @@ virStreamRecvAll(virStreamPtr stream,
virCheckNonNullArgGoto(handler, cleanup);
if (stream->flags & VIR_STREAM_NONBLOCK) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("data sinks cannot be used for non-blocking
streams"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("data sinks cannot be used for non-blocking
streams"));
goto cleanup;
}
@@ -18190,8 +18180,8 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
conn = snapshot->domain->conn;
if ((conn->flags & VIR_CONNECT_RO) && (flags &
VIR_DOMAIN_XML_SECURE)) {
- virLibConnError(VIR_ERR_OPERATION_DENIED, "%s",
- _("virDomainSnapshotGetXMLDesc with secure flag"));
+ virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("virDomainSnapshotGetXMLDesc with secure flag"));
goto error;
}
@@ -19930,8 +19920,8 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckNonNullArgGoto(cb, error);
if (conn->closeCallback->callback) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("A close callback is already registered"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A close callback is already registered"));
goto error;
}
@@ -19983,8 +19973,8 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
virCheckNonNullArgGoto(cb, error);
if (conn->closeCallback->callback != cb) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("A different callback was requested"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A different callback was requested"));
goto error;
}
@@ -20273,8 +20263,8 @@ virDomainGetCPUStats(virDomainPtr domain,
virCheckNullArgGoto(params, error);
if (nparams && ncpus > UINT_MAX / nparams) {
- virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u * %u"),
- nparams, ncpus);
+ virReportError(VIR_ERR_OVERFLOW, _("input too large: %u * %u"),
+ nparams, ncpus);
goto error;
}
if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
--
1.8.4.2
8:32 a.m.
On 01/14/2014 04:43 PM, Eric Blake wrote:
v2 of my patch series started here, after applying all patches
already reviewed in that thread:
https://www.redhat.com/archives/libvir-list/2013-December/msg01284.html
Patches 1-3 can be considered bug fixes (particularly patch 3),
so I'd like them in 1.2.1 if they get a favorable review. Patches
4-6 are less urgent, but might as well finish the work all within
one release.
Patch 1 comes from 5/24 in v1
Patch 2 is new
Patch 3 and 4 come from 23/24 in v1
Patch 5 is new
Patch 6 comes from 24/24 in v1
Eric Blake (6):
maint: don't leave garbage on early API exit
maint: avoid nested use of virConnect{Ref,Close}
maint: don't lose error on canceled migration
maint: clean up error reporting in migration
maint: simplify driver registration at startup
maint: replace remaining virLib*Error with better names
cfg.mk | 10 --
src/libvirt.c | 397 ++++++++++++++++++++-----------------------
src/lxc/lxc_process.c | 11 +-
src/qemu/qemu_driver.c | 12 +-
src/qemu/qemu_migration.c | 14 +-
src/qemu/qemu_process.c | 10 +-
src/storage/storage_driver.c | 5 +-
src/uml/uml_driver.c | 3 +-
8 files changed, 209 insertions(+), 253 deletions(-)
Patch 3 seems to be the one where there would be a couple of migration
fixes that could use a backport, especially the change in
virDomainMigrateVersion3Full() where the code now actually honors the
condition where the uri was not set rather than just playing dumb. I
also keep looking the changed "single" (!ret) to a "multi-state"
(!ret
&& !xxx) condition and keep asking myself could we run into a situation
where (!ret and xxx) is true where previously we'd fail on the !ret, but
now wouldn't because xxx was true (where xxx is retcode or cancelled).
Another "step" to consider for patch 5 would be to make common the
repetitive code that follows the virDriverCheckTabMaxReturn(), e.g.
VIR_DEBUG('string')
table[count] = driver
return count++
I don't see a reason other than proximity to release date to preclude
inclusion in 1.2.1
ACK series in general though.
John
3:07 p.m.
On 01/15/2014 07:32 AM, John Ferlan wrote:
On 01/14/2014 04:43 PM, Eric Blake wrote:
> v2 of my patch series started here, after applying all patches
> already reviewed in that thread:
> https://www.redhat.com/archives/libvir-list/2013-December/msg01284.html
>
> Patches 1-3 can be considered bug fixes (particularly patch 3),
> so I'd like them in 1.2.1 if they get a favorable review. Patches
> 4-6 are less urgent, but might as well finish the work all within
> one release.
>
> Patch 1 comes from 5/24 in v1
and I squashed in the fix pointed out by Jan.
Patch 3 seems to be the one where there would be a couple of migration
fixes that could use a backport, especially the change in
virDomainMigrateVersion3Full() where the code now actually honors the
condition where the uri was not set rather than just playing dumb. I
also keep looking the changed "single" (!ret) to a "multi-state"
(!ret
&& !xxx) condition and keep asking myself could we run into a situation
where (!ret and xxx) is true where previously we'd fail on the !ret, but
now wouldn't because xxx was true (where xxx is retcode or cancelled).
Yes - the scenario is as follows:
pre-patch:
public entry point virDomainMigrate()
-> static virDomainMigrateVersion2()
-> during prepare phase, fails to get URI, dispatches an error then
goes to finish
-> in finish phase, calls domainMigrateFinish2(cancelled = 1)
-> MigrateFinish2() will return NULL (because cancelled was set -
if it doesn't return NULL, that's a bug, where I added VIR_WARN to flag
it), but without setting an error
-> because it returns NULL, we goto the error label
-> the error label tries to dispatch the error
post-patch, we realize that if we are calling MigrateFinish2() on the
cleanup path of an aborted migration attempt (if cancelled/retcode is
non-zero), then the error message earlier in the 5-step process that
caused migration to be cancelled is the important error, and we really
don't care what happens in the finish step. That is, we want to return
NULL without raising an error if we know we are calling the finish step
on a cleanup path, and only use the 'goto error' label in the API when
we are in the finish step with all earlier steps of migration successful
so far.
Another "step" to consider for patch 5 would be to make common the
repetitive code that follows the virDriverCheckTabMaxReturn(), e.g.
VIR_DEBUG('string')
table[count] = driver
return count++
I didn't think it was worth the further compression.
I don't see a reason other than proximity to release date to preclude
inclusion in 1.2.1
So more to the point, should I push now, or wait until post-release?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:01 p.m.
On 01/15/2014 04:07 PM, Eric Blake wrote:
On 01/15/2014 07:32 AM, John Ferlan wrote:
>
> I don't see a reason other than proximity to release date to preclude
> inclusion in 1.2.1
So more to the point, should I push now, or wait until post-release?
There's bug fixes and code cleanup - it seems safe enough for me...
Still muttering to myself about the missed if (info) check... :-)
John
1:31 p.m.
On 01/15/2014 06:01 PM, John Ferlan wrote:
On 01/15/2014 04:07 PM, Eric Blake wrote:
> On 01/15/2014 07:32 AM, John Ferlan wrote:
>>
>> I don't see a reason other than proximity to release date to preclude
>> inclusion in 1.2.1
>
> So more to the point, should I push now, or wait until post-release?
>
There's bug fixes and code cleanup - it seems safe enough for me...
Still muttering to myself about the missed if (info) check... :-)
Ah well, the release happened first anyways. The series is now pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3964
days inactive
3966
days old
11 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko