[libvirt] [PATCHv2 0/6] remaining cleanups to libvirt.c
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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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
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
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
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
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko