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@xxxxxxxxx> > --- > 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
