[libvirt] [PATCH 2/2] all: replacing virGetLastError with virHas/GetLastErrorCode/Domain

2018-05-03 Thread ramyelkest
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 as well.

So Instead we create functions virGetLastErrorCode and virGetLastErrorDomain
(in addition to the existing virGetLastErrorMessage) that always return a
valid error code/domain/message, to simplify callers.

Also created virHasLastErrorCode for completion

for:
https://wiki.libvirt.org/page/BiteSizedTasks#Add_and_use_virGetLastErrorCode.28.29

Notes:
* There are a few instances of virGetLastErrorCode() left where we use multiple
fields from the error, which makes sense to keep it.
* I didn't manage to use virGetLastErrorDomain() (due to the above) so I'm 
inclined
to remove it.

Signed-off-by: Ramy Elkest 
---
 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 |  2 +-
 src/remote/remote_driver.c  |  3 +--
 src/rpc/virnetclient.c  |  2 +-
 src/util/virfilecache.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 +-
 23 files changed, 37 insertions(+), 51 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..81709d5 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 (!virHasLastError())
 rc = wantReboot ? 1 : 0;
 
  cleanup:
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 4df1bde..0d7a484 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 (!virHasLastError())
 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..7df1520 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 (!virHasLastError())
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("domain is no longer running"));
 return 

Re: [libvirt] [PATCH 2/2] all: replacing virGetLastError with virHas/GetLastErrorCode/Domain

2018-05-04 Thread Erik Skultety
On Fri, May 04, 2018 at 04:46:48AM +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 as well.
>
> So Instead we create functions virGetLastErrorCode and virGetLastErrorDomain
> (in addition to the existing virGetLastErrorMessage) that always return a
> valid error code/domain/message, to simplify callers.
>
> Also created virHasLastErrorCode for completion
>
> for:
> https://wiki.libvirt.org/page/BiteSizedTasks#Add_and_use_virGetLastErrorCode.28.29
>
> Notes:
> * There are a few instances of virGetLastErrorCode() left where we use 
> multiple
> fields from the error, which makes sense to keep it.
> * I didn't manage to use virGetLastErrorDomain() (due to the above) so I'm 
> inclined
> to remove it.

Same comments to the commit message as for patch 1.

...
>
> Signed-off-by: Ramy Elkest 
> ---
>  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 |  2 +-
>  src/remote/remote_driver.c  |  3 +--
>  src/rpc/virnetclient.c  |  2 +-
>  src/util/virfilecache.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 +-
>  23 files changed, 37 insertions(+), 51 deletions(-)

I found 2 more occurrences in src/util/virnetlibsshsession.c (l.502) and
src/util/virmodule.c (l.126) respectively, which can be optimized too.

...
>  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());

Good catch, although, this should be a separate patch IMO, let's say patch 1
and then the rest of the series. On a side note, I looked whether there aren't
other places which could use the virGetLastErrorMessage, sadly no, since the
interface isn't really flexible (among other problems with the whole error
subsystem) so that when there's no error we report "unknown error" which is
good as nothing, so providing a custom string in case there's no error or we
can't identify it would probably have been a smarter choice, oh well...

Other than that I'm fine with the changes (v2 shouldn't be a problem).

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list