[libvirt] [PATCH v2 0/3] adding virGetLastErrorCode/Domain to paritally replace virGetLastError
Changes from v1[1]: * removed virHasLastError() and s/virHasLastError/virGetLastErrorCode/g * replaced in missed files: virmodule.c and virnetlibsshsession.c * better split of patches [1] https://www.redhat.com/archives/libvir-list/2018-May/msg00259.html ramyelkest (3): util: cleanup: using virGetLastErrorMessage instead of err->message util: added virGetLastErrorCode/Domain all: replacing virGetLastError with virGetLastErrorCode where we can include/libvirt/virterror.h | 2 ++ src/libvirt_public.syms | 6 ++++++ src/locking/lock_driver_lockd.c | 3 +-- src/lxc/lxc_controller.c | 4 +--- src/qemu/qemu_agent.c | 3 +-- src/qemu/qemu_conf.c | 3 +-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++------ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_monitor.c | 5 ++--- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/remote/remote_driver.c | 3 +-- src/rpc/virnetclient.c | 2 +- src/rpc/virnetlibsshsession.c | 4 +--- src/util/virerror.c | 42 +++++++++++++++++++++++++++++++++++++++++ src/util/virfilecache.c | 3 +-- src/util/virmodule.c | 3 +-- src/util/virxml.c | 4 ++-- tests/commandtest.c | 2 +- tests/testutils.c | 6 ++---- tests/virhostcputest.c | 2 +- tests/virstoragetest.c | 8 ++++---- tools/virsh-domain-monitor.c | 7 +++---- tools/virsh-domain.c | 4 +--- tools/virsh-util.c | 3 +-- tools/vsh.c | 2 +- 28 files changed, 90 insertions(+), 57 deletions(-) -- 2.7.4
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- src/util/virfilecache.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c index dab7216..96ae96d 100644 --- a/src/util/virfilecache.c +++ b/src/util/virfilecache.c @@ -157,9 +157,8 @@ virFileCacheLoad(virFileCachePtr cache, } if (!(loadData = cache->handlers.loadFile(file, name, cache->priv))) { - virErrorPtr err = virGetLastError(); VIR_WARN("Failed to load cached data from '%s' for '%s': %s", - file, name, err ? NULLSTR(err->message) : "unknown error"); + file, name, virGetLastErrorMessage()); virResetLastError(); ret = 0; goto cleanup; -- 2.7.4
On Sat, May 05, 2018 at 01:04:19PM +0100, ramyelkest wrote:
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- src/util/virfilecache.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c index dab7216..96ae96d 100644 --- a/src/util/virfilecache.c +++ b/src/util/virfilecache.c @@ -157,9 +157,8 @@ virFileCacheLoad(virFileCachePtr cache, }
if (!(loadData = cache->handlers.loadFile(file, name, cache->priv))) { - virErrorPtr err = virGetLastError(); VIR_WARN("Failed to load cached data from '%s' for '%s': %s", - file, name, err ? NULLSTR(err->message) : "unknown error"); + file, name, virGetLastErrorMessage()); virResetLastError(); ret = 0; goto cleanup; --
Reviewed-by: Erik Skultety <eskultet@redhat.com> I'll tweak the subject line a bit before pushing: "util: Prefer virGetLastErrorMessage over direct err->message usage"
2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, May 05, 2018 at 01:04:19PM +0100, ramyelkest wrote:
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com>
There is something odd with your git setup. Did you type the signoff manually? Please set the name in your git config [0] to your legal name. That way you can certify your compliance with the DCO [1] by adding '-s' to your 'git commit' command. That way the name and e-mail in the generated signoff will match the name and e-mail in the author field, which is a requirement to get patches merged in libvirt. Jano [0] https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup [1] https://developercertificate.org/
Many places in the code call virGetLastError() just to check the raised error code, or domain. However virGetLastError() can return NULL, so the code has to check for that first. This patch therefore introduces virGetLasErrorCode/Domain function which always returns a valid error code/domain respectively, thus dropping the need to perform any checks on the error object. Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- include/libvirt/virterror.h | 2 ++ src/libvirt_public.syms | 6 ++++++ src/util/virerror.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 3e7c7a0..5e58b6a 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -344,6 +344,8 @@ void virResetLastError (void); void virResetError (virErrorPtr err); void virFreeError (virErrorPtr err); +int virGetLastErrorCode (void); +int virGetLastErrorDomain (void); const char * virGetLastErrorMessage (void); virErrorPtr virConnGetLastError (virConnectPtr conn); diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0..85b6b6d 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -785,4 +785,10 @@ LIBVIRT_4.1.0 { virStoragePoolLookupByTargetPath; } LIBVIRT_3.9.0; +LIBVIRT_4.4.0 { + global: + virGetLastErrorCode; + virGetLastErrorDomain; +} LIBVIRT_4.1.0; + # .... define new API here using predicted next version number .... diff --git a/src/util/virerror.c b/src/util/virerror.c index c000b00..842fc49 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -272,6 +272,48 @@ virGetLastError(void) /** + * virGetLastErrorCode: + * + * Get the most recent error code + * + * The error object is kept in thread local storage, so separate + * threads can safely access this concurrently. + * + * Returns the most recent error code in this thread, + * or VIR_ERR_OK if none is set + */ +int +virGetLastErrorCode(void) +{ + virErrorPtr err = virLastErrorObject(); + if (!err) + return VIR_ERR_OK; + return err->code; +} + + +/** + * virGetLastErrorDomain: + * + * Get the most recent error domain + * + * The error object is kept in thread local storage, so separate + * threads can safely access this concurrently. + * + * Returns the most recent error code in this thread, + * or VIR_FROM_NONE if none is set + */ +int +virGetLastErrorDomain(void) +{ + virErrorPtr err = virLastErrorObject(); + if (!err) + return VIR_FROM_NONE; + return err->domain; +} + + +/** * virGetLastErrorMessage: * * Get the most recent error message -- 2.7.4
On Sat, May 05, 2018 at 01:04:20PM +0100, ramyelkest wrote:
Many places in the code call virGetLastError() just to check the raised error code, or domain. However virGetLastError() can return NULL, so the code has to check for that first. This patch therefore introduces virGetLasErrorCode/Domain function which always returns a valid error code/domain respectively, thus dropping the need to perform any checks on the error object.
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- include/libvirt/virterror.h | 2 ++ src/libvirt_public.syms | 6 ++++++ src/util/virerror.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+)
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 3e7c7a0..5e58b6a 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -344,6 +344,8 @@ void virResetLastError (void); void virResetError (virErrorPtr err); void virFreeError (virErrorPtr err);
+int virGetLastErrorCode (void); +int virGetLastErrorDomain (void); const char * virGetLastErrorMessage (void);
virErrorPtr virConnGetLastError (virConnectPtr conn); diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0..85b6b6d 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -785,4 +785,10 @@ LIBVIRT_4.1.0 { virStoragePoolLookupByTargetPath; } LIBVIRT_3.9.0;
+LIBVIRT_4.4.0 { + global: + virGetLastErrorCode; + virGetLastErrorDomain; +} LIBVIRT_4.1.0; + # .... define new API here using predicted next version number .... diff --git a/src/util/virerror.c b/src/util/virerror.c index c000b00..842fc49 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -272,6 +272,48 @@ virGetLastError(void)
/** + * virGetLastErrorCode: + * + * Get the most recent error code + * + * The error object is kept in thread local storage, so separate + * threads can safely access this concurrently.
A tiny detail I missed during v1, we don't really need to mention ^this bit, we'd only explicitly document if an API wasn't thread safe, the internal facts should be transparent to users, it should *just* work, same applies to the hunk below, I'd suggest squashing in the following so that you don't have to send a v3: diff --git a/src/util/virerror.c b/src/util/virerror.c index 842fc493f1..93632dbdf7 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -274,13 +274,9 @@ virGetLastError(void) /** * virGetLastErrorCode: * - * Get the most recent error code + * Get the most recent error code (enum virErrorNumber). * - * The error object is kept in thread local storage, so separate - * threads can safely access this concurrently. - * - * Returns the most recent error code in this thread, - * or VIR_ERR_OK if none is set + * Returns the most recent error code, or VIR_ERR_OK if none is set. */ int virGetLastErrorCode(void) @@ -295,13 +291,10 @@ virGetLastErrorCode(void) /** * virGetLastErrorDomain: * - * Get the most recent error domain + * Get the most recent error domain (enum virErrorDomain). * - * The error object is kept in thread local storage, so separate - * threads can safely access this concurrently. - * - * Returns the most recent error code in this thread, - * or VIR_FROM_NONE if none is set + * Returns a numerical value of the most recent error's origin, or VIR_FROM_NONE + * if none is set. */ int virGetLastErrorDomain(void) Other than that, you have my Reviewed-by: Erik Skultety <eskultet@redhat.com>
Replaced instances where we called virGetLastError just to either get the code or to check if an error exists with virGetLastErrorCode to avoid a validity pre-check. Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- src/locking/lock_driver_lockd.c | 3 +-- src/lxc/lxc_controller.c | 4 +--- src/qemu/qemu_agent.c | 3 +-- src/qemu/qemu_conf.c | 3 +-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++------ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_monitor.c | 5 ++--- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/remote/remote_driver.c | 3 +-- src/rpc/virnetclient.c | 2 +- src/rpc/virnetlibsshsession.c | 4 +--- src/util/virmodule.c | 3 +-- src/util/virxml.c | 4 ++-- tests/commandtest.c | 2 +- tests/testutils.c | 6 ++---- tests/virhostcputest.c | 2 +- tests/virstoragetest.c | 8 ++++---- tools/virsh-domain-monitor.c | 7 +++---- tools/virsh-domain.c | 4 +--- tools/virsh-util.c | 3 +-- tools/vsh.c | 2 +- 24 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index c3fc18a..957a963 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -302,8 +302,7 @@ static int virLockManagerLockDaemonSetupLockspace(const char *path) 0, NULL, NULL, NULL, (xdrproc_t)xdr_virLockSpaceProtocolCreateLockSpaceArgs, (char*)&args, (xdrproc_t)xdr_void, NULL) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->code == VIR_ERR_OPERATION_INVALID) { + if (virGetLastErrorCode() == VIR_ERR_OPERATION_INVALID) { /* The lockspace already exists */ virResetLastError(); rv = 0; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index d5636b8..e1ee864 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1297,7 +1297,6 @@ static void virLXCControllerConsoleIO(int watch, int fd, int events, void *opaqu */ static int virLXCControllerMain(virLXCControllerPtr ctrl) { - virErrorPtr err; int rc = -1; size_t i; @@ -1349,8 +1348,7 @@ static int virLXCControllerMain(virLXCControllerPtr ctrl) virNetDaemonRun(ctrl->daemon); - err = virGetLastError(); - if (!err || err->code == VIR_ERR_OK) + if (!virGetLastErrorCode()) rc = wantReboot ? 1 : 0; cleanup: diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 4df1bde..1fb31a7 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -686,8 +686,7 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) /* Already have an error, so clear any new error */ virResetLastError(); } else { - virErrorPtr err = virGetLastError(); - if (!err) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error while processing monitor IO")); virCopyLastError(&mon->lastError); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bfbb572..a09532d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -297,8 +297,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (privileged && virFileFindHugeTLBFS(&cfg->hugetlbfs, &cfg->nhugetlbfs) < 0) { /* This however is not implemented on all platforms. */ - virErrorPtr err = virGetLastError(); - if (err && err->code != VIR_ERR_NO_SUPPORT) + if (virGetLastErrorCode() != VIR_ERR_NO_SUPPORT) goto error; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 727ea33..128f1a0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6063,7 +6063,7 @@ int qemuDomainObjExitMonitor(virQEMUDriverPtr driver, { qemuDomainObjExitMonitorInternal(driver, obj); if (!virDomainObjIsActive(obj)) { - if (!virGetLastError()) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("domain is no longer running")); return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 83fc191..bd776e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1929,7 +1929,7 @@ static int qemuDomainResume(virDomainPtr dom) if (qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_UNPAUSED, QEMU_ASYNC_JOB_NONE) < 0) { - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); goto endjob; @@ -3209,7 +3209,7 @@ qemuFileWrapperFDClose(virDomainObjPtr vm, ret = virFileWrapperFdClose(fd); virObjectLock(vm); if (!virDomainObjIsActive(vm)) { - if (!virGetLastError()) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("domain is no longer running")); ret = -1; @@ -3991,7 +3991,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); } @@ -6638,7 +6638,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_RESTORED, asyncJob) < 0) { - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); goto cleanup; @@ -14084,7 +14084,7 @@ qemuDomainSnapshotCreateActiveInternal(virQEMUDriverPtr driver, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); - if (virGetLastError() == NULL) { + if (!virGetLastErrorCode()) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after snapshot failed")); } @@ -15048,7 +15048,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); qemuDomainEventQueue(driver, event); - if (virGetLastError() == NULL) { + if (!virGetLastErrorCode()) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resuming after snapshot failed")); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9ca8e66..c755aab 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -212,7 +212,7 @@ qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, if (rc > 0) { /* the caller called qemuMonitorEjectMedia which usually reports an * error. Report the failure in an off-chance that it didn't. */ - if (!virGetLastError()) { + if (!virGetLastErrorCode()) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("timed out waiting for disk tray status update")); } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7602a30..bf4a144 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4944,7 +4944,7 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY : VIR_DOMAIN_RUNNING_MIGRATED, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) { - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); /* Need to save the current error, in case shutting @@ -5082,7 +5082,7 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, /* Set a special error if Finish is expected to return NULL as a result of * successful call with retcode != 0 */ - if (retcode != 0 && !dom && !virGetLastError()) + if (retcode != 0 && !dom && !virGetLastErrorCode()) virReportError(VIR_ERR_MIGRATE_FINISH_OK, NULL); return dom; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3918791..f68fd47 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -752,8 +752,7 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) /* Already have an error, so clear any new error */ virResetLastError(); } else { - virErrorPtr err = virGetLastError(); - if (!err) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error while processing monitor IO")); virCopyLastError(&mon->lastError); @@ -1032,7 +1031,7 @@ qemuMonitorClose(qemuMonitorPtr mon) /* Propagate existing monitor error in case the current thread has no * error set. */ - if (mon->lastError.code != VIR_ERR_OK && !virGetLastError()) + if (mon->lastError.code != VIR_ERR_OK && !virGetLastErrorCode()) virSetError(&mon->lastError); virObjectUnlock(mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8176175..306067b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4338,7 +4338,7 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, } /* Guarantee an error when returning NULL, but don't override a * more specific error if one was already generated. */ - if (!ret && !virGetLastError()) + if (!ret && !virGetLastErrorCode()) virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to find backing name for device %s"), device); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 37876b8..b8f195c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -558,7 +558,7 @@ qemuProcessFakeReboot(void *opaque) if (qemuProcessStartCPUs(driver, vm, reason, QEMU_ASYNC_JOB_NONE) < 0) { - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); goto endjob; @@ -6259,7 +6259,7 @@ qemuProcessFinishStartup(virQEMUDriverPtr driver, if (qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_BOOTED, asyncJob) < 0) { - if (!virGetLastError()) + if (!virGetLastErrorCode()) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 95437b4..e670e16 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3672,8 +3672,7 @@ remoteAuthenticate(virConnectPtr conn, struct private_data *priv, (xdrproc_t) xdr_void, (char *) NULL, (xdrproc_t) xdr_remote_auth_list_ret, (char *) &ret); if (err < 0) { - virErrorPtr verr = virGetLastError(); - if (verr && verr->code == VIR_ERR_NO_SUPPORT) { + if (virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) { /* Missing RPC - old server - ignore */ virResetLastError(); return 0; diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 6bbc984..b4d8fb2 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -1958,7 +1958,7 @@ static int virNetClientIO(virNetClientPtr client, virNetClientIOUpdateCallback(client, true); if (rv == 0 && - virGetLastError()) + virGetLastErrorCode()) rv = -1; cleanup: diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 309e8a9..b2cb5c1 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -499,9 +499,7 @@ virNetLibsshImportPrivkey(virNetLibsshSessionPtr sess, err = SSH_AUTH_ERROR; goto error; } else if (ret == SSH_ERROR) { - virErrorPtr vir_err = virGetLastError(); - - if (!vir_err || !vir_err->code) { + if (!virGetLastErrorCode()) { virReportError(VIR_ERR_AUTH_FAILED, _("error while opening private key '%s', wrong " "passphrase?"), diff --git a/src/util/virmodule.c b/src/util/virmodule.c index ff8c227..9d7d249 100644 --- a/src/util/virmodule.c +++ b/src/util/virmodule.c @@ -123,8 +123,7 @@ virModuleLoad(const char *path, if ((*regsym)() < 0) { /* regsym() should report an error itself, but lets * just make sure */ - virErrorPtr err = virGetLastError(); - if (err == NULL) { + if (!virGetLastErrorCode()) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to execute symbol '%s' in module '%s'"), regfunc, path); diff --git a/src/util/virxml.c b/src/util/virxml.c index 6e87605..b5caee7 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -708,7 +708,7 @@ catchXMLError(void *ctx, const char *msg ATTRIBUTE_UNUSED, ...) /* conditions for error printing */ if (!ctxt || - (virGetLastError() != NULL) || + (virGetLastErrorCode()) || ctxt->input == NULL || ctxt->lastError.level != XML_ERR_FATAL || ctxt->lastError.message == NULL) @@ -845,7 +845,7 @@ virXMLParseHelper(int domcode, xmlFreeDoc(xml); xml = NULL; - if (virGetLastError() == NULL) { + if (!virGetLastErrorCode()) { virGenericReportError(domcode, VIR_ERR_XML_ERROR, "%s", _("failed to parse xml document")); } diff --git a/tests/commandtest.c b/tests/commandtest.c index ad81c2a..3bb7220 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -127,7 +127,7 @@ static int test0(const void *unused ATTRIBUTE_UNUSED) if (virCommandRun(cmd, NULL) == 0) goto cleanup; - if (virGetLastError() == NULL) + if (!virGetLastErrorCode()) goto cleanup; virResetLastError(); diff --git a/tests/testutils.c b/tests/testutils.c index 4b13d11..f0719c8 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -178,8 +178,7 @@ virTestRun(const char *title, virResetLastError(); ret = body(data); - virErrorPtr err = virGetLastError(); - if (err) { + if (virGetLastErrorCode()) { if (virTestGetVerbose() || virTestGetDebug()) virDispatchError(NULL); } @@ -258,8 +257,7 @@ virTestRun(const char *title, fprintf(stderr, " alloc %zu failed but no err status\n", i + 1); # endif } else { - virErrorPtr lerr = virGetLastError(); - if (!lerr) { + if (!virGetLastErrorCode()) { # if 0 fprintf(stderr, " alloc %zu failed but no error report\n", i + 1); # endif diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c index cb318df..091dd59 100644 --- a/tests/virhostcputest.c +++ b/tests/virhostcputest.c @@ -49,7 +49,7 @@ linuxTestCompareFiles(const char *cpuinfofile, &nodeinfo.nodes, &nodeinfo.sockets, &nodeinfo.cores, &nodeinfo.threads) < 0) { if (virTestGetDebug()) { - if (virGetLastError()) + if (virGetLastErrorCode()) VIR_TEST_DEBUG("\n%s\n", virGetLastErrorMessage()); } VIR_FORCE_FCLOSE(cpuinfo); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 0e11602..de68ea0 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -333,7 +333,7 @@ testStorageChain(const void *args) goto cleanup; } if (data->flags & EXP_WARN) { - if (!virGetLastError()) { + if (!virGetLastErrorCode()) { fprintf(stderr, "call should have warned\n"); goto cleanup; } @@ -343,7 +343,7 @@ testStorageChain(const void *args) goto cleanup; } } else { - if (virGetLastError()) { + if (virGetLastErrorCode()) { fprintf(stderr, "call should not have warned\n"); goto cleanup; } @@ -449,13 +449,13 @@ testStorageLookup(const void *args) idx, NULL); if (!data->expResult) { - if (!virGetLastError()) { + if (!virGetLastErrorCode()) { fprintf(stderr, "call should have failed\n"); ret = -1; } virResetLastError(); } else { - if (virGetLastError()) { + if (virGetLastErrorCode()) { fprintf(stderr, "call should not have warned\n"); ret = -1; } diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8e07177..32099cb 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -63,7 +63,6 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, unsigned int flags) { char *desc = NULL; - virErrorPtr err = NULL; xmlDocPtr doc = NULL; xmlXPathContextPtr ctxt = NULL; int type; @@ -76,15 +75,15 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, if ((desc = virDomainGetMetadata(dom, type, NULL, flags))) { return desc; } else { - err = virGetLastError(); + int errCode = virGetLastErrorCode(); - if (err && err->code == VIR_ERR_NO_DOMAIN_METADATA) { + if (errCode == VIR_ERR_NO_DOMAIN_METADATA) { desc = vshStrdup(ctl, ""); vshResetLibvirtError(); return desc; } - if (err && err->code != VIR_ERR_NO_SUPPORT) + if (errCode != VIR_ERR_NO_SUPPORT) return desc; } diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 598d2fa..e9972b9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -94,9 +94,7 @@ virshDomainDefine(virConnectPtr conn, const char *xml, unsigned int flags) * try again. */ if (!dom) { - virErrorPtr err = virGetLastError(); - if (err && - (err->code == VIR_ERR_NO_SUPPORT) && + if ((virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) && (flags == VIR_DOMAIN_DEFINE_VALIDATE)) dom = virDomainDefineXML(conn, xml); } diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad..aa88397 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -123,8 +123,7 @@ virshDomainState(vshControl *ctl, if (!priv->useGetInfo) { int state; if (virDomainGetState(dom, &state, reason, 0) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->code == VIR_ERR_NO_SUPPORT) + if (virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) priv->useGetInfo = true; else return -1; diff --git a/tools/vsh.c b/tools/vsh.c index 73ec007..8f8ffb0 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -266,7 +266,7 @@ vshSaveLibvirtHelperError(void) if (last_error) return; - if (!virGetLastError()) + if (!virGetLastErrorCode()) return; vshSaveLibvirtError(); -- 2.7.4
On Sat, May 05, 2018 at 01:04:21PM +0100, ramyelkest wrote:
Replaced instances where we called virGetLastError just to either get the code or to check if an error exists with virGetLastErrorCode to avoid a validity pre-check.
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com> --- src/locking/lock_driver_lockd.c | 3 +-- src/lxc/lxc_controller.c | 4 +--- src/qemu/qemu_agent.c | 3 +-- src/qemu/qemu_conf.c | 3 +-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++------ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_monitor.c | 5 ++--- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/remote/remote_driver.c | 3 +-- src/rpc/virnetclient.c | 2 +- src/rpc/virnetlibsshsession.c | 4 +--- src/util/virmodule.c | 3 +-- src/util/virxml.c | 4 ++-- tests/commandtest.c | 2 +- tests/testutils.c | 6 ++---- tests/virhostcputest.c | 2 +- tests/virstoragetest.c | 8 ++++---- tools/virsh-domain-monitor.c | 7 +++---- tools/virsh-domain.c | 4 +--- tools/virsh-util.c | 3 +-- tools/vsh.c | 2 +- 24 files changed, 39 insertions(+), 55 deletions(-) ...
- err = virGetLastError(); - if (!err || err->code == VIR_ERR_OK) + if (!virGetLastErrorCode()) rc = wantReboot ? 1 : 0;
Again, just a tiny detail, the convention we use in libvirt with unary '!' is that we only use it with pointers, but explicitly compare against an int otherwise, so ^this would become == VIR_ERR_OK (yes, we don't mention this in our hacking guidelines...). So hopefully I didn't miss anything in the attached diff which I'd squash in before pushing, feel free to check again. The replacement itself is fine, so Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Sat, May 05, 2018 at 01:04:18PM +0100, ramyelkest wrote:
Changes from v1[1]:
* removed virHasLastError() and s/virHasLastError/virGetLastErrorCode/g * replaced in missed files: virmodule.c and virnetlibsshsession.c * better split of patches
[1] https://www.redhat.com/archives/libvir-list/2018-May/msg00259.html
ramyelkest (3): util: cleanup: using virGetLastErrorMessage instead of err->message util: added virGetLastErrorCode/Domain all: replacing virGetLastError with virGetLastErrorCode where we can
I suggested some minor adjustments to your patches without the need for a v3, so let me know if you agree and I'll merge the series. Erik
participants (3)
-
Erik Skultety -
Ján Tomko -
ramyelkest