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

[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 <ramyelkest at 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         |  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