[libvirt] [PATCH 0/2] *** use virGetLastErrorMessage ***
*** BLURB HERE *** Jovanka Gulicoska (2): tests: use virGetLastErrorMessage() Use virGetLastErrorMessage()
--- tests/commandtest.c | 81 ++++++++++++++-------------------------- tests/libvirtdconftest.c | 26 ++++++------- tests/openvzutilstest.c | 7 +--- tests/securityselinuxlabeltest.c | 6 +-- tests/securityselinuxtest.c | 6 +-- tests/virpolkittest.c | 18 ++++----- 6 files changed, 55 insertions(+), 89 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index cf5f44a..6430e20 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -178,8 +178,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) int ret; if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -190,8 +189,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED) } if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -218,8 +216,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) VIR_COMMAND_PASS_FD_CLOSE_PARENT); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -260,8 +257,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED) virCommandDaemonize(cmd); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -294,8 +290,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED) virCommandAddEnvPassCommon(cmd); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -318,8 +313,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -343,8 +337,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -368,8 +361,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED) virCommandAddEnvPair(cmd, "USER", "test"); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -406,8 +398,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED) } if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -432,8 +423,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED) virCommandAddArgSet(cmd, args); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -456,8 +446,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED) virCommandPtr cmd = virCommandNewArgs(args); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -478,8 +467,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED) virCommandSetInputBuffer(cmd, "Hello World\n"); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); virCommandFree(cmd); return -1; } @@ -506,8 +494,7 @@ static int test13(const void *unused ATTRIBUTE_UNUSED) virCommandSetOutputBuffer(cmd, &outactual); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (!outactual) @@ -559,8 +546,7 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) virCommandSetErrorBuffer(cmd, &erractual); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (!outactual || !erractual) @@ -573,8 +559,7 @@ static int test14(const void *unused ATTRIBUTE_UNUSED) virCommandSetOutputBuffer(cmd, &jointactual); virCommandSetErrorBuffer(cmd, &jointactual); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (!jointactual) @@ -620,8 +605,7 @@ static int test15(const void *unused ATTRIBUTE_UNUSED) virCommandSetUmask(cmd, 002); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -651,8 +635,7 @@ static int test16(const void *unused ATTRIBUTE_UNUSED) virCommandAddArg(cmd, "G H"); if ((outactual = virCommandToString(cmd)) == NULL) { - virErrorPtr err = virGetLastError(); - printf("Cannot convert to string: %s\n", err->message); + printf("Cannot convert to string: %s\n", virGetLastErrorMessage()); goto cleanup; } if ((fd = open(abs_builddir "/commandhelper.log", @@ -697,8 +680,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) } if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -720,8 +702,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) } if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -756,8 +737,7 @@ static int test18(const void *unused ATTRIBUTE_UNUSED) alarm(5); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } alarm(0); @@ -798,8 +778,7 @@ static int test19(const void *unused ATTRIBUTE_UNUSED) alarm(5); if (virCommandRunAsync(cmd, &pid) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -848,8 +827,7 @@ static int test20(const void *unused ATTRIBUTE_UNUSED) virCommandSetInputBuffer(cmd, buf); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -891,8 +869,7 @@ static int test21(const void *unused ATTRIBUTE_UNUSED) virCommandDoAsyncIO(cmd); if (virCommandRunAsync(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -930,8 +907,7 @@ test22(const void *unused ATTRIBUTE_UNUSED) cmd = virCommandNewArgList("/bin/sh", "-c", "exit 3", NULL); if (virCommandRun(cmd, &status) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (status != 3) { @@ -941,8 +917,7 @@ test22(const void *unused ATTRIBUTE_UNUSED) virCommandRawStatus(cmd); if (virCommandRun(cmd, &status) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (!WIFEXITED(status) || WEXITSTATUS(status) != 3) { @@ -960,8 +935,7 @@ test22(const void *unused ATTRIBUTE_UNUSED) virCommandRawStatus(cmd); if (virCommandRun(cmd, &status) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) { @@ -1057,8 +1031,7 @@ static int test24(const void *unused ATTRIBUTE_UNUSED) virCommandPassListenFDs(cmd); if (virCommandRun(cmd, NULL) < 0) { - virErrorPtr err = virGetLastError(); - printf("Cannot run child %s\n", err->message); + printf("Cannot run child %s\n", virGetLastErrorMessage()); goto cleanup; } diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index 61d861d..daa2b90 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -102,7 +102,7 @@ testCorrupt(const void *opaque) data->params, data->paramnum, &type); - virErrorPtr err = NULL; + const char *err = NULL; if (!newdata) return -1; @@ -115,15 +115,15 @@ testCorrupt(const void *opaque) goto cleanup; } - err = virGetLastError(); - if (!err || !err->message) { + err = virGetLastErrorMessage(); + if (!err) { VIR_DEBUG("No error or message %p", err); ret = -1; goto cleanup; } #if !WITH_SASL - if (strstr(err->message, "unsupported auth sasl")) { + if (strstr(err, "unsupported auth sasl")) { VIR_DEBUG("sasl unsupported, skipping this config"); goto cleanup; } @@ -131,24 +131,24 @@ testCorrupt(const void *opaque) switch (type) { case VIR_CONF_ULONG: - if (!strstr(err->message, "invalid type: got string; expected unsigned long") && - !strstr(err->message, "invalid type: got string; expected long")) { + if (!strstr(err, "invalid type: got string; expected unsigned long") && + !strstr(err, "invalid type: got string; expected long")) { VIR_DEBUG("Wrong error for long: '%s'", - err->message); + err); ret = -1; } break; case VIR_CONF_STRING: - if (!strstr(err->message, "invalid type: got unsigned long; expected string")) { + if (!strstr(err, "invalid type: got unsigned long; expected string")) { VIR_DEBUG("Wrong error for string: '%s'", - err->message); + err); ret = -1; } break; case VIR_CONF_LIST: - if (!strstr(err->message, "must be a string or list of strings")) { + if (!strstr(err, "must be a string or list of strings")) { VIR_DEBUG("Wrong error for list: '%s'", - err->message); + err); ret = -1; } break; @@ -212,8 +212,8 @@ mymain(void) } if (virFileReadAll(filename, 1024*1024, &filedata) < 0) { - virErrorPtr err = virGetLastError(); - fprintf(stderr, "Cannot load %s for testing: %s", filename, err->message); + const char *err = virGetLastErrorMessage(); + fprintf(stderr, "Cannot load %s for testing: %s", filename, err); ret = -1; goto cleanup; } diff --git a/tests/openvzutilstest.c b/tests/openvzutilstest.c index ccde636..d747165 100644 --- a/tests/openvzutilstest.c +++ b/tests/openvzutilstest.c @@ -75,7 +75,6 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED) int result = -1; virDomainDefPtr def = NULL; char *actual = NULL; - virErrorPtr err = NULL; const char *expected = "<domain type='openvz'>\n" " <uuid>00000000-0000-0000-0000-000000000000</uuid>\n" @@ -110,16 +109,14 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED) def->os.type = VIR_DOMAIN_OSTYPE_EXE; if (openvzReadNetworkConf(def, 1) < 0) { - err = virGetLastError(); - fprintf(stderr, "ERROR: %s\n", err != NULL ? err->message : "<unknown>"); + fprintf(stderr, "ERROR: %s\n", virGetLastErrorMessage()); goto cleanup; } actual = virDomainDefFormat(def, NULL, VIR_DOMAIN_DEF_FORMAT_INACTIVE); if (actual == NULL) { - err = virGetLastError(); - fprintf(stderr, "ERROR: %s\n", err != NULL ? err->message : "<unknown>"); + fprintf(stderr, "ERROR: %s\n", virGetLastErrorMessage()); goto cleanup; } diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index c82b3f2..f6caa30 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -332,8 +332,7 @@ testSELinuxLabeling(const void *opaque) } VIR_FREE(files); if (ret < 0) { - virErrorPtr err = virGetLastError(); - VIR_TEST_VERBOSE("%s\n", err ? err->message : "<unknown>"); + VIR_TEST_VERBOSE("%s\n", virGetLastErrorMessage()); } return ret; } @@ -354,9 +353,8 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { - virErrorPtr err = virGetLastError(); VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n", - err->message); + virGetLastErrorMessage()); return EXIT_FAILURE; } diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index 49694f3..3423e66 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -230,8 +230,7 @@ testSELinuxGenLabel(const void *opaque) goto cleanup; if (virSecurityManagerGenLabel(data->mgr, def) < 0) { - virErrorPtr err = virGetLastError(); - fprintf(stderr, "Cannot generate label: %s\n", err->message); + fprintf(stderr, "Cannot generate label: %s\n", virGetLastErrorMessage()); goto cleanup; } @@ -275,9 +274,8 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { - virErrorPtr err = virGetLastError(); fprintf(stderr, "Unable to initialize security driver: %s\n", - err->message); + virGetLastErrorMessage()); return EXIT_FAILURE; } diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c index 73f001b..151fd6e 100644 --- a/tests/virpolkittest.c +++ b/tests/virpolkittest.c @@ -169,7 +169,7 @@ static int testPolkitAuthDenied(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; int rv; - virErrorPtr err; + const char *err; rv = virPolkitCheckAuth("org.libvirt.test.deny", THE_PID, @@ -185,8 +185,8 @@ static int testPolkitAuthDenied(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - err = virGetLastError(); - if (!err || !strstr(err->message, + err = virGetLastErrorMessage(); + if (!err || !strstr(err, _("access denied by policy"))) { fprintf(stderr, "Incorrect error response\n"); goto cleanup; @@ -238,7 +238,7 @@ static int testPolkitAuthCancelled(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; int rv; - virErrorPtr err; + const char *err; rv = virPolkitCheckAuth("org.libvirt.test.cancelled", THE_PID, @@ -254,8 +254,8 @@ static int testPolkitAuthCancelled(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - err = virGetLastError(); - if (!err || !strstr(err->message, + err = virGetLastErrorMessage(); + if (!err || !strstr(err, _("user cancelled authentication process"))) { fprintf(stderr, "Incorrect error response\n"); goto cleanup; @@ -295,7 +295,7 @@ static int testPolkitAuthDetailsDenied(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; int rv; - virErrorPtr err; + const char *err; const char *details[] = { "org.libvirt.test.person", "Joe", NULL, @@ -315,8 +315,8 @@ static int testPolkitAuthDetailsDenied(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - err = virGetLastError(); - if (!err || !strstr(err->message, + err = virGetLastErrorMessage(); + if (!err || !strstr(err, _("access denied by policy"))) { fprintf(stderr, "Incorrect error response\n"); goto cleanup; -- 2.5.5
On 05/10/2016 08:16 AM, Jovanka Gulicoska wrote:
--- tests/commandtest.c | 81 ++++++++++++++-------------------------- tests/libvirtdconftest.c | 26 ++++++------- tests/openvzutilstest.c | 7 +--- tests/securityselinuxlabeltest.c | 6 +-- tests/securityselinuxtest.c | 6 +-- tests/virpolkittest.c | 18 ++++----- 6 files changed, 55 insertions(+), 89 deletions(-)
There's a couple other test instances - tests/qemucapsprobe.c - tests/nodeinfo.c:linuxTestCompareFiles : you'll want to do if (virGetLastError()) VIR_TEST_DEBUG("\n%s\n", error->message); to preserve the behavior Comments inline
diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index 61d861d..daa2b90 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -102,7 +102,7 @@ testCorrupt(const void *opaque) data->params, data->paramnum, &type); - virErrorPtr err = NULL; + const char *err = NULL;
if (!newdata) return -1; @@ -115,15 +115,15 @@ testCorrupt(const void *opaque) goto cleanup; }
- err = virGetLastError(); - if (!err || !err->message) { + err = virGetLastErrorMessage(); + if (!err) { VIR_DEBUG("No error or message %p", err); ret = -1; goto cleanup; }
virGetLastErrorMessage() doesn't return NULL, so this check will never trigger. I think it's better to just not change libvirtdconftest.c. Same with virpolkittest.c which is similar Thanks, Cole
virGetLastErrorMessage() in daemon, examples,bhyve, lxc, logging, locking, libvirt, libxl, lxc, rpc, storage, uml, and util. Fix syntax error in tests. --- daemon/libvirtd.c | 8 ++------ examples/object-events/event-test.c | 9 +++------ src/bhyve/bhyve_driver.c | 3 +-- src/libvirt.c | 3 +-- src/libxl/libxl_domain.c | 3 +-- src/libxl/libxl_driver.c | 4 +--- src/locking/lock_daemon.c | 8 ++------ src/logging/log_daemon.c | 8 ++------ src/lxc/lxc_container.c | 9 +++------ src/lxc/lxc_controller.c | 9 +++------ src/lxc/lxc_domain.c | 4 ++-- src/lxc/lxc_process.c | 6 ++---- src/rpc/virnettlscontext.c | 3 +-- src/storage/storage_driver.c | 16 ++++------------ src/uml/uml_driver.c | 3 +-- src/util/iohelper.c | 10 ++-------- src/util/virhook.c | 3 +-- src/util/virhostdev.c | 20 ++++++++------------ tests/securityselinuxlabeltest.c | 4 ++-- 19 files changed, 42 insertions(+), 91 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index cc5190f..32c6655 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1315,12 +1315,8 @@ int main(int argc, char **argv) { /* Read the config file if it exists*/ if (remote_config_file && daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - VIR_ERROR(_("Can't load config file: %s: %s"), - err->message, remote_config_file); - else - VIR_ERROR(_("Can't load config file: %s"), remote_config_file); + VIR_ERROR(_("Can't load config file: %s: %s"), + virGetLastErrorMessage(), remote_config_file); exit(EXIT_FAILURE); } diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index 2063536..c1ff4a7 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -917,9 +917,8 @@ main(int argc, char **argv) } if (virEventRegisterDefaultImpl() < 0) { - virErrorPtr err = virGetLastError(); fprintf(stderr, "Failed to register event implementation: %s\n", - err && err->message ? err->message: "Unknown error"); + virGetLastErrorMessage()); goto cleanup; } @@ -972,17 +971,15 @@ main(int argc, char **argv) goto cleanup; if (virConnectSetKeepAlive(dconn, 5, 3) < 0) { - virErrorPtr err = virGetLastError(); fprintf(stderr, "Failed to start keepalive protocol: %s\n", - err && err->message ? err->message : "Unknown error"); + virGetLastErrorMessage()); run = 0; } while (run) { if (virEventRunDefaultImpl() < 0) { - virErrorPtr err = virGetLastError(); fprintf(stderr, "Failed to run event loop: %s\n", - err && err->message ? err->message : "Unknown error"); + virGetLastErrorMessage()); } } diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 4fc504e..ddfcbc8 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -88,9 +88,8 @@ bhyveAutostartDomain(virDomainObjPtr vm, void *opaque) ret = virBhyveProcessStart(data->conn, data->driver, vm, VIR_DOMAIN_RUNNING_BOOTED, 0); if (ret < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), - vm->def->name, err ? err->message : _("unknown error")); + vm->def->name, virGetLastErrorMessage()); } } virObjectUnlock(vm); diff --git a/src/libvirt.c b/src/libvirt.c index 114e88c..0e7e435 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -770,10 +770,9 @@ virStateInitialize(bool privileged, if (virStateDriverTab[i]->stateInitialize(privileged, callback, opaque) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Initialization of %s state driver failed: %s"), virStateDriverTab[i]->name, - err && err->message ? err->message : _("Unknown problem")); + virGetLastErrorMessage()); return -1; } } diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 14a900c..8128525 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -515,9 +515,8 @@ libxlDomainShutdownThread(void *opaque) libxlDomainDestroyInternal(driver, vm); libxlDomainCleanup(driver, vm); if (libxlDomainStart(driver, vm, false, -1) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to restart VM '%s': %s"), - vm->def->name, err ? err->message : _("unknown error")); + vm->def->name, virGetLastErrorMessage()); } endjob: diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index bf97c9c..e58bc22 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -311,7 +311,6 @@ libxlAutostartDomain(virDomainObjPtr vm, void *opaque) { libxlDriverPrivatePtr driver = opaque; - virErrorPtr err; int ret = -1; virObjectLock(vm); @@ -324,10 +323,9 @@ libxlAutostartDomain(virDomainObjPtr vm, if (vm->autostart && !virDomainObjIsActive(vm) && libxlDomainStart(driver, vm, false, -1) < 0) { - err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, - err ? err->message : _("unknown error")); + virGetLastErrorMessage()); goto endjob; } diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index f889a34..2c45349 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1264,12 +1264,8 @@ int main(int argc, char **argv) { /* Read the config file if it exists*/ if (remote_config_file && virLockDaemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - VIR_ERROR(_("Can't load config file: %s: %s"), - err->message, remote_config_file); - else - VIR_ERROR(_("Can't load config file: %s"), remote_config_file); + VIR_ERROR(_("Can't load config file: %s: %s"), + virGetLastErrorMessage(), remote_config_file); exit(EXIT_FAILURE); } diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 90f8427..8f1ccc2 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -1021,12 +1021,8 @@ int main(int argc, char **argv) { /* Read the config file if it exists*/ if (remote_config_file && virLogDaemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - VIR_ERROR(_("Can't load config file: %s: %s"), - err->message, remote_config_file); - else - VIR_ERROR(_("Can't load config file: %s"), remote_config_file); + VIR_ERROR(_("Can't load config file: %s: %s"), + virGetLastErrorMessage(), remote_config_file); exit(EXIT_FAILURE); } diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index a909b66..33dcfec 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2290,12 +2290,9 @@ static int lxcContainerChild(void *data) if (ret != 0) { VIR_DEBUG("Tearing down container"); - virErrorPtr err = virGetLastError(); - if (err && err->message) - fprintf(stderr, "%s\n", err->message); - else - fprintf(stderr, "%s\n", - _("Unknown failure in libvirt_lxc startup")); + fprintf(stderr, + _("Failure in libvirt_lxc startup: %s\n"), + virGetLastErrorMessage()); } virCommandFree(cmd); diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 0304354..dc3921b 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2736,12 +2736,9 @@ int main(int argc, char *argv[]) cleanup: if (rc < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - fprintf(stderr, "%s\n", err->message); - else - fprintf(stderr, "%s\n", - _("Unknown failure in libvirt_lxc startup")); + fprintf(stderr, + _("Failure in libvirt_lxc startup: %s\n"), + virGetLastErrorMessage()); } virPidFileDelete(LXC_STATE_DIR, name); diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 3177a62..57fd170 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -221,8 +221,8 @@ virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, unsigned long long thepid; if (virXPathULongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) { - virErrorPtr err = virGetLastError(); - VIR_WARN("Failed to load init pid from state %s", err ? err->message : "null"); + VIR_WARN("Failed to load init pid from state %s", + virGetLastErrorMessage()); priv->initpid = 0; } else { priv->initpid = thepid; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0044ee5..5164535 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -755,10 +755,9 @@ static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED priv->initpid = initpid; if (virLXCProcessGetNsInode(initpid, "pid", &inode) < 0) { - virErrorPtr err = virGetLastError(); VIR_WARN("Cannot obtain pid NS inode for %llu: %s", (unsigned long long)initpid, - err && err->message ? err->message : "<unknown>"); + virGetLastErrorMessage()); virResetLastError(); } virDomainAuditInit(vm, initpid, inode); @@ -1617,10 +1616,9 @@ virLXCProcessAutostartDomain(virDomainObjPtr vm, VIR_DOMAIN_RUNNING_BOOTED); virDomainAuditStart(vm, "booted", ret >= 0); if (ret < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, - err ? err->message : ""); + virGetLastErrorMessage()); } else { virObjectEventPtr event = virDomainEventLifecycleNewFromObj(vm, diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 947038d..6e78623 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1141,8 +1141,7 @@ int virNetTLSContextCheckCertificate(virNetTLSContextPtr ctxt, virObjectLock(ctxt); virObjectLock(sess); if (virNetTLSContextValidCertificate(ctxt, sess) < 0) { - virErrorPtr err = virGetLastError(); - VIR_WARN("Certificate check failed %s", err && err->message ? err->message : "<unknown>"); + VIR_WARN("Certificate check failed %s", virGetLastErrorMessage()); if (ctxt->requireValidCert) { virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Failed to verify peer's certificate")); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1d42f24..38e90c0 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -98,10 +98,8 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) active = false; if (backend->checkPool && backend->checkPool(pool, &active) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); + pool->def->name, virGetLastErrorMessage()); goto error; } @@ -112,12 +110,10 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) if (active) { virStoragePoolObjClearVols(pool); if (backend->refreshPool(NULL, pool) < 0) { - virErrorPtr err = virGetLastError(); if (backend->stopPool) backend->stopPool(NULL, pool); VIR_ERROR(_("Failed to restart storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); + pool->def->name, virGetLastErrorMessage()); goto error; } } @@ -176,10 +172,8 @@ storageDriverAutostart(void) !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(conn, pool) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); + pool->def->name, virGetLastErrorMessage()); virStoragePoolObjUnlock(pool); continue; } @@ -195,14 +189,12 @@ storageDriverAutostart(void) if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { - virErrorPtr err = virGetLastError(); if (stateFile) unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); + pool->def->name, virGetLastErrorMessage()); } else { pool->active = true; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 84e1df8..923c3f6 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -188,9 +188,8 @@ umlAutostartDomain(virDomainObjPtr vm, ret = umlStartVMDaemon(data->conn, data->driver, vm, false); virDomainAuditStart(vm, "booted", ret >= 0); if (ret < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), - vm->def->name, err ? err->message : _("unknown error")); + vm->def->name, virGetLastErrorMessage()); } else { virObjectEventPtr event = virDomainEventLifecycleNewFromObj(vm, diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 275f993..00f31e7 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -218,7 +218,6 @@ int main(int argc, char **argv) { const char *path; - virErrorPtr err; unsigned long long offset; unsigned long long length; int oflags = -1; @@ -303,12 +302,7 @@ main(int argc, char **argv) return 0; error: - err = virGetLastError(); - if (err) { - fprintf(stderr, "%s: %s\n", program_name, err->message); - } else { - fprintf(stderr, _("%s: unknown failure with %s\n"), - program_name, path); - } + fprintf(stderr, _("%s: failure with %s\n: %s"), + program_name, path, virGetLastErrorMessage()); exit(EXIT_FAILURE); } diff --git a/src/util/virhook.c b/src/util/virhook.c index ba50598..d37d6da 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -297,9 +297,8 @@ virHookCall(int driver, ret = virCommandRun(cmd, NULL); if (ret < 0) { /* Convert INTERNAL_ERROR into known error. */ - virErrorPtr err = virGetLastError(); virReportError(VIR_ERR_HOOK_SCRIPT_FAILED, "%s", - err ? err->message : _("unknown error")); + virGetLastErrorMessage()); } virCommandFree(cmd); diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 933c942..e90c2df 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -801,10 +801,9 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); + virGetLastErrorMessage()); + virResetLastError(); } } @@ -829,10 +828,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virObjectLock(mgr->inactivePCIHostdevs); if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate PCI device list: %s"), - err ? err->message : _("unknown error")); - virResetError(err); + virGetLastErrorMessage()); + virResetLastError(); goto cleanup; } @@ -883,10 +881,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, if (!actual || virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to add PCI device %s to the inactive list"), - err ? err->message : _("unknown error")); - virResetError(err); + virGetLastErrorMessage()); + virResetLastError(); } } @@ -928,10 +925,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { - virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s"), - err ? err->message : _("unknown error")); - virResetError(err); + virGetLastErrorMessage()); + virResetLastError(); } } diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index f6caa30..0f6cb66 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -331,9 +331,9 @@ testSELinuxLabeling(const void *opaque) VIR_FREE(files[i].context); } VIR_FREE(files); - if (ret < 0) { + if (ret < 0) VIR_TEST_VERBOSE("%s\n", virGetLastErrorMessage()); - } + return ret; } -- 2.5.5
On 05/10/2016 08:16 AM, Jovanka Gulicoska wrote:
virGetLastErrorMessage() in daemon, examples,bhyve, lxc, logging, locking, libvirt, libxl, lxc, rpc, storage, uml, and util.
Fix syntax error in tests.
Patches should be individually clean, so the test fixup should have been squashed into the previous patch. Easiest way to do that: make the changes, git commit tests/, then git rebase -i and 'fixup' the previous commit. This patch also conflicts with changes in the libxl driver that have gone since you posted, so be careful with that Additionally there's another instance that needs fixing in src/conf/virsecretobj.c Otherwise the patch looks good Thanks, Cole
On 10/05/16 14:16, Jovanka Gulicoska wrote:
*** BLURB HERE ***
Hi, thank you for patches :), just a small advice to the future, we tend to write a couple of words/sentences, instead of the "BLURB HERE", what the series actually does. Also, the subject could be slightly more verbose and the asterisks are unnecessary. Anyway, I've seen some minor issues in the patches, but those really were just details like indentation being off in 2 places I think, on specific places virGetLastErrorMessage could be used directly as an argument to another function, instead of first assigning the result of this function to a variable and then using that variable as an argument to another function without any other usage for that variable, and maybe slightly more descriptive commit messages, but I went through them only briefly... The reason why I'm replying to this mail is that regardless of the patches being correct or not, I would like to postpone accepting these patches until the virGetLastErrorMessage function is fixed. I know that the original idea comes from our wiki page http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMes... and the patches followed this example http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html. But it wasn't until I reviewed https://www.redhat.com/archives/libvir-list/2016-May/msg00516.html that I realized that there is a problem with virGetLastErrorMessage and cannot be used as a replacement for virGetLastError until it's fixed. So I'd like to express my concerns once again. Basically, the problem lies within calling virLastErrorObject from both virGetLastError and virGetLastErrorMessage functions. It's a slight misinterpretation from our side that if virLastErrorObject returns NULL it means that no error occurred - wrong, virLastErrorObject would always return a valid object stating that there was or wasn't an error, while NULL means that either it failed to allocate a new thread-local error object or it failed to set this newly allocated thread-local object. But while virGetLastError handles this just by passing the pointer back to the original caller and they need to check for it (err && err->message), virGetLastErrorMessage on the other hand would just return "no error" in this case and the user would get a message like "internal error: no error" which surely looks odd. Now, I know that patch 1/2 also fixes an issue with our tests where a NULL dereference might occur and should be fixed, but that isn't the case for libvirt's code. This reply was meant to provide some details for the next reviewer, so they could take that into account when reviewing, but from my perspective, we can wait a little longer until we sort things out in the virterror module first, namely in the virGetLastErrorMessage function. Regards, Erik
On 05/10/2016 11:44 AM, Erik Skultety wrote:
On 10/05/16 14:16, Jovanka Gulicoska wrote:
*** BLURB HERE ***
Hi, thank you for patches :), just a small advice to the future, we tend to write a couple of words/sentences, instead of the "BLURB HERE", what the series actually does. Also, the subject could be slightly more verbose and the asterisks are unnecessary.
Anyway, I've seen some minor issues in the patches, but those really were just details like indentation being off in 2 places I think, on specific places virGetLastErrorMessage could be used directly as an argument to another function, instead of first assigning the result of this function to a variable and then using that variable as an argument to another function without any other usage for that variable, and maybe slightly more descriptive commit messages, but I went through them only briefly...
The reason why I'm replying to this mail is that regardless of the patches being correct or not, I would like to postpone accepting these patches until the virGetLastErrorMessage function is fixed. I know that the original idea comes from our wiki page http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMes... and the patches followed this example http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html. But it wasn't until I reviewed https://www.redhat.com/archives/libvir-list/2016-May/msg00516.html that I realized that there is a problem with virGetLastErrorMessage and cannot be used as a replacement for virGetLastError until it's fixed. So I'd like to express my concerns once again. Basically, the problem lies within calling virLastErrorObject from both virGetLastError and virGetLastErrorMessage functions. It's a slight misinterpretation from our side that if virLastErrorObject returns NULL it means that no error occurred - wrong, virLastErrorObject would always return a valid object stating that there was or wasn't an error, while NULL means that either it failed to allocate a new thread-local error object or it failed to set this newly allocated thread-local object. But while virGetLastError handles this just by passing the pointer back to the original caller and they need to check for it (err && err->message), virGetLastErrorMessage on the other hand would just return "no error" in this case and the user would get a message like "internal error: no error" which surely looks odd.
Now, I know that patch 1/2 also fixes an issue with our tests where a NULL dereference might occur and should be fixed, but that isn't the case for libvirt's code. This reply was meant to provide some details for the next reviewer, so they could take that into account when reviewing, but from my perspective, we can wait a little longer until we sort things out in the virterror module first, namely in the virGetLastErrorMessage function.
Hmmmm. So to summarize, the cases that virGetLastErrorMessage() will return "no error" when a possible error actually occurred is: - virLastErrorObject VIR_ALLOC_QUIET(err) fails - virLastErrorObject virThreadLocalSet fails virThreadLocalSet is just pthread_setspecific, which the only error code that's documented is ENOMEM. So it's basically the same as the ALLOC failure. Maybe have virGetLastErrorMessage() save errno and check for ENOMEM after calling virLastErrorObject(), but I'm not really sure it's worth the complexity: once we get to the point of malloc failing I assume functionality is going to degrade rapidly, so a less accurate error isn't going to be impactful in that case. What are your thoughts Erik? Thanks, Cole
On 10/05/16 18:51, Cole Robinson wrote:
On 05/10/2016 11:44 AM, Erik Skultety wrote:
On 10/05/16 14:16, Jovanka Gulicoska wrote:
*** BLURB HERE ***
Hi, thank you for patches :), just a small advice to the future, we tend to write a couple of words/sentences, instead of the "BLURB HERE", what the series actually does. Also, the subject could be slightly more verbose and the asterisks are unnecessary.
Anyway, I've seen some minor issues in the patches, but those really were just details like indentation being off in 2 places I think, on specific places virGetLastErrorMessage could be used directly as an argument to another function, instead of first assigning the result of this function to a variable and then using that variable as an argument to another function without any other usage for that variable, and maybe slightly more descriptive commit messages, but I went through them only briefly...
The reason why I'm replying to this mail is that regardless of the patches being correct or not, I would like to postpone accepting these patches until the virGetLastErrorMessage function is fixed. I know that the original idea comes from our wiki page http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMes... and the patches followed this example http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html. But it wasn't until I reviewed https://www.redhat.com/archives/libvir-list/2016-May/msg00516.html that I realized that there is a problem with virGetLastErrorMessage and cannot be used as a replacement for virGetLastError until it's fixed. So I'd like to express my concerns once again. Basically, the problem lies within calling virLastErrorObject from both virGetLastError and virGetLastErrorMessage functions. It's a slight misinterpretation from our side that if virLastErrorObject returns NULL it means that no error occurred - wrong, virLastErrorObject would always return a valid object stating that there was or wasn't an error, while NULL means that either it failed to allocate a new thread-local error object or it failed to set this newly allocated thread-local object. But while virGetLastError handles this just by passing the pointer back to the original caller and they need to check for it (err && err->message), virGetLastErrorMessage on the other hand would just return "no error" in this case and the user would get a message like "internal error: no error" which surely looks odd.
Now, I know that patch 1/2 also fixes an issue with our tests where a NULL dereference might occur and should be fixed, but that isn't the case for libvirt's code. This reply was meant to provide some details for the next reviewer, so they could take that into account when reviewing, but from my perspective, we can wait a little longer until we sort things out in the virterror module first, namely in the virGetLastErrorMessage function.
Hmmmm. So to summarize, the cases that virGetLastErrorMessage() will return "no error" when a possible error actually occurred is:
- virLastErrorObject VIR_ALLOC_QUIET(err) fails - virLastErrorObject virThreadLocalSet fails
virThreadLocalSet is just pthread_setspecific, which the only error code that's documented is ENOMEM. So it's basically the same as the ALLOC failure.
Maybe have virGetLastErrorMessage() save errno and check for ENOMEM after calling virLastErrorObject(), but I'm not really sure it's worth the complexity: once we get to the point of malloc failing I assume functionality is going to degrade rapidly, so a less accurate error isn't going to be impactful in that case.
What are your thoughts Erik?
Thanks, Cole
I have to admit that I didn't know that pthread_setspecific returned ENOMEM. Sure, once malloc fails, either being called directly from our code or indirectly from a system library, incorrect message would be the least of our concerns. Hmm, you're right about saving the errno, it probably wouldn't be worth it. How about just tweaking the conditions like this (the complexity of the change equals one more operand and a change in logical operation): diff --git a/src/util/virerror.c b/src/util/virerror.c index 5d875e3..1177570 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -281,9 +281,9 @@ const char * virGetLastErrorMessage(void) { virErrorPtr err = virLastErrorObject(); - if (!err || err->code == VIR_ERR_OK) + if (err && err->code == VIR_ERR_OK) return _("no error"); - if (err->message == NULL) + if (!err || !err->message) return _("unknown error"); return err->message; } Compared to virGetLastError which returns NULL when malloc failed (one way or the other) and we just format it as "Unknown error", the behaviour would remain the same with the above proposed change. What do you think? Erik
On 05/11/2016 07:45 AM, Erik Skultety wrote:
On 10/05/16 18:51, Cole Robinson wrote:
On 05/10/2016 11:44 AM, Erik Skultety wrote:
On 10/05/16 14:16, Jovanka Gulicoska wrote:
*** BLURB HERE ***
Hi, thank you for patches :), just a small advice to the future, we tend to write a couple of words/sentences, instead of the "BLURB HERE", what the series actually does. Also, the subject could be slightly more verbose and the asterisks are unnecessary.
Anyway, I've seen some minor issues in the patches, but those really were just details like indentation being off in 2 places I think, on specific places virGetLastErrorMessage could be used directly as an argument to another function, instead of first assigning the result of this function to a variable and then using that variable as an argument to another function without any other usage for that variable, and maybe slightly more descriptive commit messages, but I went through them only briefly...
The reason why I'm replying to this mail is that regardless of the patches being correct or not, I would like to postpone accepting these patches until the virGetLastErrorMessage function is fixed. I know that the original idea comes from our wiki page http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMes... and the patches followed this example http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html. But it wasn't until I reviewed https://www.redhat.com/archives/libvir-list/2016-May/msg00516.html that I realized that there is a problem with virGetLastErrorMessage and cannot be used as a replacement for virGetLastError until it's fixed. So I'd like to express my concerns once again. Basically, the problem lies within calling virLastErrorObject from both virGetLastError and virGetLastErrorMessage functions. It's a slight misinterpretation from our side that if virLastErrorObject returns NULL it means that no error occurred - wrong, virLastErrorObject would always return a valid object stating that there was or wasn't an error, while NULL means that either it failed to allocate a new thread-local error object or it failed to set this newly allocated thread-local object. But while virGetLastError handles this just by passing the pointer back to the original caller and they need to check for it (err && err->message), virGetLastErrorMessage on the other hand would just return "no error" in this case and the user would get a message like "internal error: no error" which surely looks odd.
Now, I know that patch 1/2 also fixes an issue with our tests where a NULL dereference might occur and should be fixed, but that isn't the case for libvirt's code. This reply was meant to provide some details for the next reviewer, so they could take that into account when reviewing, but from my perspective, we can wait a little longer until we sort things out in the virterror module first, namely in the virGetLastErrorMessage function.
Hmmmm. So to summarize, the cases that virGetLastErrorMessage() will return "no error" when a possible error actually occurred is:
- virLastErrorObject VIR_ALLOC_QUIET(err) fails - virLastErrorObject virThreadLocalSet fails
virThreadLocalSet is just pthread_setspecific, which the only error code that's documented is ENOMEM. So it's basically the same as the ALLOC failure.
Maybe have virGetLastErrorMessage() save errno and check for ENOMEM after calling virLastErrorObject(), but I'm not really sure it's worth the complexity: once we get to the point of malloc failing I assume functionality is going to degrade rapidly, so a less accurate error isn't going to be impactful in that case.
What are your thoughts Erik?
Thanks, Cole
I have to admit that I didn't know that pthread_setspecific returned ENOMEM. Sure, once malloc fails, either being called directly from our code or indirectly from a system library, incorrect message would be the least of our concerns. Hmm, you're right about saving the errno, it probably wouldn't be worth it. How about just tweaking the conditions like this (the complexity of the change equals one more operand and a change in logical operation):
diff --git a/src/util/virerror.c b/src/util/virerror.c index 5d875e3..1177570 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -281,9 +281,9 @@ const char * virGetLastErrorMessage(void) { virErrorPtr err = virLastErrorObject(); - if (!err || err->code == VIR_ERR_OK) + if (err && err->code == VIR_ERR_OK) return _("no error"); - if (err->message == NULL) + if (!err || !err->message) return _("unknown error"); return err->message; }
Compared to virGetLastError which returns NULL when malloc failed (one way or the other) and we just format it as "Unknown error", the behaviour would remain the same with the above proposed change. What do you think?
Good idea, I think that's an improvement. Thanks, Cole
On 05/10/2016 08:16 AM, Jovanka Gulicoska wrote:
*** BLURB HERE ***
Jovanka Gulicoska (2): tests: use virGetLastErrorMessage() Use virGetLastErrorMessage()
Like Erik pointed out, you missed the blurb, and the subject shouldn't contain ***. Look to the list traffic for examples of how other cover letters look Thanks, Cole
participants (3)
-
Cole Robinson -
Erik Skultety -
Jovanka Gulicoska