From 727eab8a576b998d94ba03fd4295fcdafa05f5c3 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 13:29:34 +0100 Subject: [PATCH 01/22] tests/Makefile.am: memcheck: report OVERALL FAILED cases (if any) Signed-off-by: Jim Klimov --- tests/Makefile.am | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 48beeac5fd..a4cf0dd63d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -192,13 +192,17 @@ endif !HAVE_CXX11 if HAVE_VALGRIND # NOTE: "cppnit", if built, requires running from NIT (with NUT_PORT, etc.) +# Note that FAILED value begins with a space, so we do not echo another memcheck: $(check_PROGRAMS) - RES=0; for P in $? ; do \ + @RES=0; FAILED=""; \ + for P in $? ; do \ case "$$P" in \ cppnit|cppnit$(EXEEXT)) if [ "$${NUT_PORT-}" -gt 0 ] 2>/dev/null ; then : ; else echo "SKIPPED: $(VALGRIND) ./$$P : NUT_PORT not prepared" ; continue ; fi ;; \ esac ; \ - $(VALGRIND) ./$$P || { RES=$$? ; echo "FAILED: $(VALGRIND) ./$$P" >&2; } ; \ - done; exit $$RES + $(VALGRIND) ./$$P || { RES=$$? ; echo "FAILED: $(VALGRIND) ./$$P" >&2; FAILED="$$FAILED $$P" ; } ; \ + done; \ + if [ -n "$$FAILED" ] ; then echo "OVERALL FAILED:$$FAILED" >&2 ; fi ; \ + exit $$RES else !HAVE_VALGRIND memcheck: @echo "SKIPPED $@ : valgrind was not detected on this system by configure script" >&2 From 47daa47b2cb169eba5692dfb1c05f7aa2771ede9 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 13:36:03 +0100 Subject: [PATCH 02/22] scripts/valgrind/valgrind.sh: allow to customize the VALGRIND to be used Signed-off-by: Jim Klimov --- scripts/valgrind/valgrind.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/valgrind/valgrind.sh b/scripts/valgrind/valgrind.sh index 2bb8608125..e17c092df5 100755 --- a/scripts/valgrind/valgrind.sh +++ b/scripts/valgrind/valgrind.sh @@ -1,6 +1,6 @@ #!/bin/sh -# Copyright (C) 2024 by Jim Klimov +# Copyright (C) 2024-2025 by Jim Klimov SCRIPTDIR="`dirname "$0"`" SCRIPTDIR="`cd "$SCRIPTDIR" && pwd`" @@ -8,11 +8,12 @@ SCRIPTDIR="`cd "$SCRIPTDIR" && pwd`" LTEXEC="" [ -n "${LIBTOOL-}" ] || LIBTOOL="`command -v libtool`" 2>/dev/null [ -z "${LIBTOOL}" ] || LTEXEC="${LIBTOOL} --mode=execute " +[ -n "${VALGRIND-}" ] || VALGRIND="valgrind" # TODO: Also wrap tool=callgrind e.g. via $0 symlink name? exec ${LTEXEC} \ -valgrind \ +${VALGRIND} \ --tool=memcheck --verbose \ --leak-check=full --show-reachable=yes --error-exitcode=1 --show-error-list=yes \ --trace-children=yes --track-fds=yes --show-leak-kinds=all --track-origins=yes \ From ecf489a3a0e4333241360cda3d7ec1e74ee11edb Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 13:44:43 +0100 Subject: [PATCH 03/22] tests/Makefile.am: memcheck: use the NUT-customized wrapper for valgrind Signed-off-by: Jim Klimov --- tests/Makefile.am | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index a4cf0dd63d..dff3c4881e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -197,9 +197,17 @@ memcheck: $(check_PROGRAMS) @RES=0; FAILED=""; \ for P in $? ; do \ case "$$P" in \ - cppnit|cppnit$(EXEEXT)) if [ "$${NUT_PORT-}" -gt 0 ] 2>/dev/null ; then : ; else echo "SKIPPED: $(VALGRIND) ./$$P : NUT_PORT not prepared" ; continue ; fi ;; \ + cppnit|cppnit$(EXEEXT)) \ + if [ "$${NUT_PORT-}" -gt 0 ] 2>/dev/null ; then : ; else \ + echo "SKIPPED: $(VALGRIND) ./$$P : NUT_PORT not prepared" ; \ + continue ; \ + fi ;; \ esac ; \ - $(VALGRIND) ./$$P || { RES=$$? ; echo "FAILED: $(VALGRIND) ./$$P" >&2; FAILED="$$FAILED $$P" ; } ; \ + VALGRIND='$(VALGRIND)' '$(top_srcdir)/scripts/valgrind/valgrind.sh' ./$$P \ + || { RES=$$? ; \ + echo "FAILED: VALGRIND='$(VALGRIND)' '$(top_srcdir)/scripts/valgrind/valgrind.sh' ./$$P" >&2; \ + FAILED="$$FAILED $$P" ; \ + } ; \ done; \ if [ -n "$$FAILED" ] ; then echo "OVERALL FAILED:$$FAILED" >&2 ; fi ; \ exit $$RES From 64893a744889fd69881ea7dee6fea2559a46d50b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 13:38:56 +0100 Subject: [PATCH 04/22] configure.ac, scripts/valgrind/valgrind.sh.in, tests/Makefile.am: make the helper script a template Signed-off-by: Jim Klimov --- configure.ac | 1 + scripts/valgrind/.gitignore | 1 + scripts/valgrind/README.adoc | 5 +++++ scripts/valgrind/{valgrind.sh => valgrind.sh.in} | 6 +++++- tests/Makefile.am | 4 ++-- 5 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 scripts/valgrind/.gitignore rename scripts/valgrind/{valgrind.sh => valgrind.sh.in} (73%) diff --git a/configure.ac b/configure.ac index 1b6f8a330c..fa2021ba58 100644 --- a/configure.ac +++ b/configure.ac @@ -5799,6 +5799,7 @@ m4_foreach_w([SCRIPTFILE], [ scripts/Solaris/postremove scripts/Solaris/preproto.pl scripts/Solaris/nut + scripts/valgrind/valgrind.sh tools/gitlog2changelog.py tools/nut-snmpinfo.py ], [ diff --git a/scripts/valgrind/.gitignore b/scripts/valgrind/.gitignore new file mode 100644 index 0000000000..83ee3f6e3f --- /dev/null +++ b/scripts/valgrind/.gitignore @@ -0,0 +1 @@ +/valgrind.sh diff --git a/scripts/valgrind/README.adoc b/scripts/valgrind/README.adoc index 6d5866163e..bf55b224d6 100644 --- a/scripts/valgrind/README.adoc +++ b/scripts/valgrind/README.adoc @@ -9,6 +9,11 @@ Example use-case: ./scripts/valgrind/valgrind.sh ./tools/nut-scanner/nut-scanner -DDDDDD -m auto ---- +Note that the script is generated under `${top_builddir}` by `configure` from +a template file located in `${top_srcdir}/scripts/valgrind/valgrind.sh.in`. +You might be able to run it directly, falling back to a `valgrind` program in +your `PATH`, if any. + See also: * link:https://wiki.wxwidgets.org/Valgrind_Suppression_File_Howto[Valgrind Suppression File How-to] diff --git a/scripts/valgrind/valgrind.sh b/scripts/valgrind/valgrind.sh.in similarity index 73% rename from scripts/valgrind/valgrind.sh rename to scripts/valgrind/valgrind.sh.in index e17c092df5..cc372bdac4 100755 --- a/scripts/valgrind/valgrind.sh +++ b/scripts/valgrind/valgrind.sh.in @@ -8,7 +8,11 @@ SCRIPTDIR="`cd "$SCRIPTDIR" && pwd`" LTEXEC="" [ -n "${LIBTOOL-}" ] || LIBTOOL="`command -v libtool`" 2>/dev/null [ -z "${LIBTOOL}" ] || LTEXEC="${LIBTOOL} --mode=execute " -[ -n "${VALGRIND-}" ] || VALGRIND="valgrind" + +# Allow to run un-parsed "sh ${top_srcdir}/scripts/valgrind/valgrind.sh.in" +# with reasonable success: +[ -n "${VALGRIND-}" ] || case '@VALGRIND@' in [@]*) ;; *) VALGRIND='@VALGRIND@' ;; esac +[ -n "${VALGRIND-}" ] || VALGRIND="valgrind" # TODO: Also wrap tool=callgrind e.g. via $0 symlink name? diff --git a/tests/Makefile.am b/tests/Makefile.am index dff3c4881e..38b4f71c99 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -203,9 +203,9 @@ memcheck: $(check_PROGRAMS) continue ; \ fi ;; \ esac ; \ - VALGRIND='$(VALGRIND)' '$(top_srcdir)/scripts/valgrind/valgrind.sh' ./$$P \ + '$(top_builddir)/scripts/valgrind/valgrind.sh' "./$$P" \ || { RES=$$? ; \ - echo "FAILED: VALGRIND='$(VALGRIND)' '$(top_srcdir)/scripts/valgrind/valgrind.sh' ./$$P" >&2; \ + echo "FAILED: '$(top_builddir)/scripts/valgrind/valgrind.sh' './$$P'" >&2; \ FAILED="$$FAILED $$P" ; \ } ; \ done; \ From 7f25581086cc5448babeeefc918909284a2f8557 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 14:09:52 +0100 Subject: [PATCH 05/22] scripts/valgrind/valgrind.sh.in: comment about --gen-suppressions=all option Signed-off-by: Jim Klimov --- scripts/valgrind/valgrind.sh.in | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/valgrind/valgrind.sh.in b/scripts/valgrind/valgrind.sh.in index cc372bdac4..4719d6646a 100755 --- a/scripts/valgrind/valgrind.sh.in +++ b/scripts/valgrind/valgrind.sh.in @@ -1,6 +1,8 @@ #!/bin/sh # Copyright (C) 2024-2025 by Jim Klimov +# NOTE: If there are system problems to suppress, re-run the test with +# --gen-suppressions=all option, and update the .valgrind.supp file. SCRIPTDIR="`dirname "$0"`" SCRIPTDIR="`cd "$SCRIPTDIR" && pwd`" From 404bc4cb8430499e73fcd0bb499321fa6b348d24 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 15:31:08 +0100 Subject: [PATCH 06/22] scripts/valgrind/.valgrind.supp: add suppressions for C++ tests (CPPUNIT leaky internals) Signed-off-by: Jim Klimov --- scripts/valgrind/.valgrind.supp | 174 ++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/scripts/valgrind/.valgrind.supp b/scripts/valgrind/.valgrind.supp index 2e175ebaf5..c37aaeb5b3 100644 --- a/scripts/valgrind/.valgrind.supp +++ b/scripts/valgrind/.valgrind.supp @@ -140,3 +140,177 @@ fun:start_thread ... } + +# Numerous reported leaks seem to be part of CPPUNIT itself, quesce them: +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::ConcretTestFixtureFactory<*>::makeFixture() + ... + #CppUnit::TestFactoryRegistry::makeTest() + fun:_ZN7CppUnit19TestFactoryRegistry8makeTestEv + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::SynchronizedObject::SynchronizedObject(CppUnit::SynchronizedObject::SynchronizationObject*) + fun:_ZN7CppUnit18SynchronizedObjectC1EPNS0_21SynchronizationObjectE + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestResult::TestResult(CppUnit::SynchronizedObject::SynchronizationObject*) + fun:_ZN7CppUnit10TestResultC1EPNS_18SynchronizedObject21SynchronizationObjectE + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestRunner::TestRunner() + fun:_ZN7CppUnit10TestRunnerC1Ev + #CppUnit::TextTestRunner::TextTestRunner(CppUnit::Outputter*) + fun:_ZN7CppUnit14TextTestRunnerC1EPNS_9OutputterE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TextTestRunner::TextTestRunner(CppUnit::Outputter*) + fun:_ZN7CppUnit14TextTestRunnerC1EPNS_9OutputterE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestResultCollector::TestResultCollector(CppUnit::SynchronizedObject::SynchronizationObject*) + fun:_ZN7CppUnit19TestResultCollectorC1EPNS_18SynchronizedObject21SynchronizationObjectE + #CppUnit::TextTestRunner::TextTestRunner(CppUnit::Outputter*) + fun:_ZN7CppUnit14TextTestRunnerC1EPNS_9OutputterE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestSuite::addTest(CppUnit::Test*) + fun:_ZN7CppUnit9TestSuite7addTestEPNS_4TestE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestComposite::TestComposite(std::__cxx11::basic_string, std::allocator > const&) + fun:_ZN7CppUnit13TestCompositeC1ERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::TestCase::TestCase(std::__cxx11::basic_string, std::allocator > const&) + fun:_ZN7CppUnit8TestCaseC1ERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #void std::_Function_base::_Base_manager >::_M_create >(std::_Any_data&, std::_Bind&&, std::integral_constant) + ###fun:_ZNSt14_Function_base13_Base_managerISt5_BindIFM11ExampleTestFvvEPS2_EEE9_M_createIS7_EEvRSt9_Any_dataOT_St17integral_constantIbLb0EE + fun:_ZNSt14_Function_base13_Base_managerISt5_BindIFM11*vvEPS2_EEE9_M_createIS7_EEvRSt9_Any_dataOT_St17integral_constantIbLb0EE + ... + #CppUnit::TestFactoryRegistry::addTestToSuite(CppUnit::TestSuite*) + fun:_ZN7CppUnit19TestFactoryRegistry14addTestToSuiteEPNS_9TestSuiteE +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::ProtectorChain::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) + fun:_ZN7CppUnit14ProtectorChain7protectERKNS_7FunctorERKNS_16ProtectorContextE + #CppUnit::TestResult::protect(CppUnit::Functor const&, CppUnit::Test*, std::__cxx11::basic_string, std::allocator > const&) + fun:_ZN7CppUnit10TestResult7protectERKNS_7FunctorEPNS_4TestERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + ... +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + #CppUnit::ProtectorChain::protect() + fun:_ZN7CppUnit14ProtectorChainC1Ev + #CppUnit::TestResult::TestResult(CppUnit::SynchronizedObject::SynchronizationObject*) + fun:_ZN7CppUnit10TestResultC1EPNS_18SynchronizedObject21SynchronizationObjectE + #CppUnit::TextTestRunner::TextTestRunner(CppUnit::Outputter*) + fun:_ZN7CppUnit14TextTestRunnerC1EPNS_9OutputterE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + obj:*libcppunit* + #CppUnit::TestResultCollector::TestResultCollector(CppUnit::SynchronizedObject::SynchronizationObject*) + fun:_ZN7CppUnit19TestResultCollectorC1EPNS_18SynchronizedObject21SynchronizationObjectE + #CppUnit::TextTestRunner::TextTestRunner(CppUnit::Outputter*) + fun:_ZN7CppUnit14TextTestRunnerC1EPNS_9OutputterE + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + obj:*libcppunit* + #CppUnit::TestPath::TestPath(CppUnit::Test*, std::__cxx11::basic_string, std::allocator > const&) + fun:_ZN7CppUnit8TestPathC1EPNS_4TestERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + #CppUnit::TestRunner::run(CppUnit::TestResult&, std::__cxx11::basic_string, std::allocator > const&) + fun:_ZN7CppUnit10TestRunner3runERNS_10TestResultERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + #CppUnit::TextTestRunner::run(std::__cxx11::basic_string, std::allocator >, bool, bool, bool) + fun:_ZN7CppUnit14TextTestRunner3runENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEbbb + fun:main +} +{ + + ### runner.setOutputter( new CppUnit::CompilerOutputter( ...) ) in main (cpputest.cpp:120) + Memcheck:Leak + match-leak-kinds: reachable + #operator new(unsigned long) + fun:_Znwm + fun:main +} From edc7f01ecc48b3f617f098ad2d08b07dbdcd5b41 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 15:33:49 +0100 Subject: [PATCH 07/22] scripts/valgrind/.valgrind.supp: add suppressions for dash interpreter Signed-off-by: Jim Klimov --- scripts/valgrind/.valgrind.supp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/scripts/valgrind/.valgrind.supp b/scripts/valgrind/.valgrind.supp index c37aaeb5b3..ea2866c880 100644 --- a/scripts/valgrind/.valgrind.supp +++ b/scripts/valgrind/.valgrind.supp @@ -1,3 +1,22 @@ +{ + + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + obj:/usr/bin/dash + ... + fun:(below main) +} +{ + + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + fun:strdup + obj:/usr/bin/dash + ... + fun:(below main) +} { Memcheck:Param From 84fce774df4fbcde941ced8c78d06b9b1be3e2a6 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 15:48:57 +0100 Subject: [PATCH 08/22] common/nutstream.cpp: checkExistsWritableDir(): fix memory leak with opened DIRFD Signed-off-by: Jim Klimov --- common/nutstream.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/nutstream.cpp b/common/nutstream.cpp index 01471e1cf8..d4dc867a80 100644 --- a/common/nutstream.cpp +++ b/common/nutstream.cpp @@ -197,6 +197,7 @@ NutStream::status_t NutMemory::putData(const std::string & data) { * by packager who knows their system. */ static bool checkExistsWritableDir(const char *s) { + DIR *pd; #ifdef DEBUG std::cerr << "checkExistsWritableDir(" << (s ? s : "") << "): "; #endif @@ -207,12 +208,13 @@ static bool checkExistsWritableDir(const char *s) { return false; } - if (!opendir(s)) { + if (!(pd = opendir(s))) { #ifdef DEBUG std::cerr << "not a dir" << std::endl; #endif return false; } + closedir(pd); /* POSIX: If the requested access is permitted, access() succeeds * and shall return 0; otherwise, -1 shall be returned and errno From 9760763b5745a06aa824e7b68480f56fecb22239 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 15:49:30 +0100 Subject: [PATCH 09/22] tests/nutconf_ut.cpp: NutConfigUnitTest::testUpsmonConfiguration(): fix memory leak with temporary BoolInt instances Signed-off-by: Jim Klimov --- tests/nutconf_ut.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/nutconf_ut.cpp b/tests/nutconf_ut.cpp index 4c135ae5aa..715676e4ab 100644 --- a/tests/nutconf_ut.cpp +++ b/tests/nutconf_ut.cpp @@ -196,6 +196,7 @@ void NutConfigUnitTest::testNutConfiguration() { void NutConfigUnitTest::testUpsmonConfiguration() { nut::UpsmonConfiguration config; + nut::BoolInt *tmpPtr = nullptr; // Note: this file gets generated from a .in template load(static_cast(&config), ABS_TOP_BUILDDIR "/conf/upsmon.conf.sample"); @@ -212,10 +213,10 @@ void NutConfigUnitTest::testUpsmonConfiguration() { // Hm, maybe we need typed constructors to init // the values for test? ("new" weirdness below // is due to Settable<> formal type mismatch) - config.shutdownExit = (*(new nut::BoolInt()) << "true"); + config.shutdownExit = (*(tmpPtr = new nut::BoolInt()) << "true"); delete tmpPtr; config.oblbDuration = -1; - config.certVerify = (*(new nut::BoolInt()) << false); - config.forceSsl = (*(new nut::BoolInt()) << "1"); + config.certVerify = (*(tmpPtr = new nut::BoolInt()) << false); delete tmpPtr; + config.forceSsl = (*(tmpPtr = new nut::BoolInt()) << "1"); delete tmpPtr; config.certIdent.certName = "My test cert"; config.certIdent.certDbPass = "DbPwd!"; @@ -223,14 +224,14 @@ void NutConfigUnitTest::testUpsmonConfiguration() { nut::CertHost certHost; certHost.host = "remote:3493"; certHost.certName = "NUT server cert"; - certHost.certVerify = (*(new nut::BoolInt()) << "true"); - certHost.forceSsl = (*(new nut::BoolInt()) << 0); + certHost.certVerify = (*(tmpPtr = new nut::BoolInt()) << "true"); delete tmpPtr; + certHost.forceSsl = (*(tmpPtr = new nut::BoolInt()) << 0); delete tmpPtr; config.certHosts.push_back(certHost); certHost.host = "localhost:13493"; certHost.certName = "My NUT server cert"; - certHost.certVerify = (*(new nut::BoolInt()) << "false"); - certHost.forceSsl = (*(new nut::BoolInt()) << false); + certHost.certVerify = (*(tmpPtr = new nut::BoolInt()) << "false"); delete tmpPtr; + certHost.forceSsl = (*(tmpPtr = new nut::BoolInt()) << false); delete tmpPtr; config.certHosts.push_back(certHost); // Note: More config data points come from the file loaded above From 9cf811e7860182906d7b67bcbb1143ccf9c71a17 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:26:27 +0100 Subject: [PATCH 10/22] tests/generic_gpio_utest.c: refactor work with generic_gpio_close() to avoid use after free Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index 35759d50eb..b86a0bb7e8 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -211,15 +211,15 @@ int main(int argc, char **argv) { if(fEof!=EOF) { if(!strcmp(testType, "rules")) { struct gpioups_t *upsfdtest = xcalloc(1, sizeof(*upsfdtest)); + /* NOTE: here and below, freed by generic_gpio_close(upsfdtest) */ jmp_result = setjmp(env_buffer); if(jmp_result) { /* test case exiting */ - generic_gpio_close(upsfdtest); printf("%s %s test rule %u [%s]\n", pass_fail[get_test_status(upsfdtest, 1)], testType, i, rules); } else { /* run test case */ get_ups_rules(upsfdtest, (unsigned char *)rules); - generic_gpio_close(upsfdtest); printf("%s %s test rule %u [%s]\n", pass_fail[get_test_status(upsfdtest, 0)], testType, i, rules); } + generic_gpio_close(upsfdtest); } if(!strcmp(testType, "states")) { int expectedStateValue; @@ -281,7 +281,6 @@ int main(int argc, char **argv) { jmp_result = setjmp(env_buffer); if (jmp_result) { failed=1; - generic_gpio_close(upsfdtest); } else { update_ups_states(upsfdtest); currUpsStatus=dstate_getinfo("ups.status"); @@ -292,8 +291,8 @@ int main(int argc, char **argv) { if(!strcmp(chargeStatus,".") && currChargerStatus!=NULL) failed=1; if( strcmp(chargeLow,".") && strcmp(charge,".") && (!currCharge || strcmp(currCharge, charge))) failed=1; if(!strcmp(chargeLow,".") && !strcmp(charge,".") && currCharge!=NULL) failed=1; - generic_gpio_close(upsfdtest); } + generic_gpio_close(upsfdtest); printf("%s %s test rule %u [%s] ([%s] %s %s (%s)) ([%s] %s %s)\n", pass_fail[failed], testType, i, rules, upsStatus, chargeStatus, charge, chargeLow, From 507c5b37212b3f0d8f65a02400739025a6947774 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:27:00 +0100 Subject: [PATCH 11/22] tests/generic_gpio_utest.c: comment away upsdrv_cleanup() in codepath which we did not upsdrv_init() Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index b86a0bb7e8..dd2164e882 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -336,7 +336,7 @@ int main(int argc, char **argv) { failed = expecting_failure; if(jmp_result) { /* test case exiting */ if(expecting_failure) failed=0; - upsdrv_cleanup(); + /* upsdrv_cleanup(); */ } else { if(expecting_failure) failed=1; device_path = chipNameLocal; From 7c38008ca33032b4818e5cb27507f262c58a62e7 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:27:18 +0100 Subject: [PATCH 12/22] tests/generic_gpio_utest.c: dstate_free() after the test run Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index dd2164e882..6ff14b0704 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -389,6 +389,8 @@ int main(int argc, char **argv) { fclose(testData); done = 1; + dstate_free(); + /* Return 0 (exit-code OK, boolean false) if no tests failed and some ran */ if ( (cases_failed == 0) && (cases_passed > 0) ) return 0; From ac8fd77a462a1104907c17eeb36036817978c84e Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:51:29 +0100 Subject: [PATCH 13/22] drivers/generic_gpio_common.c: fix code markup to adhere closer to NUT coding style Signed-off-by: Jim Klimov --- drivers/generic_gpio_common.c | 185 +++++++++++++++++----------------- 1 file changed, 94 insertions(+), 91 deletions(-) diff --git a/drivers/generic_gpio_common.c b/drivers/generic_gpio_common.c index c4aa8ac90a..059af237c9 100644 --- a/drivers/generic_gpio_common.c +++ b/drivers/generic_gpio_common.c @@ -35,7 +35,7 @@ struct gpioups_t *generic_gpio_open(const char *chipName); #ifndef DRIVERS_MAIN_WITHOUT_MAIN static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ -void generic_gpio_close(struct gpioups_t *gpioupsfd); +void generic_gpio_close(struct gpioups_t *gpioupsfdlocal); #ifndef DRIVERS_MAIN_WITHOUT_MAIN static @@ -60,7 +60,7 @@ int calc_rule_states(int upsLinesStates[], int cRules[], int subCount, int sInde #ifndef DRIVERS_MAIN_WITHOUT_MAIN static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ -void update_ups_states(struct gpioups_t *gpioupsfd); +void update_ups_states(struct gpioups_t *gpioupsfdlocal); /* * allocate common data structures and process/check rules @@ -69,11 +69,12 @@ void update_ups_states(struct gpioups_t *gpioupsfd); static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ struct gpioups_t *generic_gpio_open(const char *chipName) { - struct gpioups_t *upsfdlocal = xcalloc(1, sizeof(*upsfdlocal)); - upsfdlocal->runOptions=0; /* don't use ROPT_REQRES and ROPT_EVMODE yet */ - upsfdlocal->chipName=chipName; + struct gpioups_t *upsfdlocal = xcalloc(1, sizeof(*upsfdlocal)); - if(!testvar("rules")) /* rules is required configuration parameter */ + upsfdlocal->runOptions = 0; /* don't use ROPT_REQRES and ROPT_EVMODE yet */ + upsfdlocal->chipName = chipName; + + if (!testvar("rules")) /* rules is required configuration parameter */ fatalx(EXIT_FAILURE, "UPS status calculation rules not specified"); get_ups_rules(upsfdlocal, (unsigned char *)getval("rules")); @@ -97,8 +98,8 @@ void generic_gpio_close(struct gpioups_t *gpioupsfdlocal) { free(gpioupsfdlocal->upsLinesStates); } if (gpioupsfdlocal->rules) { - int i; - for (i=0; i < gpioupsfdlocal->rulesCount; i++) { + int i; + for (i = 0; i < gpioupsfdlocal->rulesCount; i++) { free(gpioupsfdlocal->rules[i]); } } @@ -114,12 +115,12 @@ void generic_gpio_close(struct gpioups_t *gpioupsfdlocal) { static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ void add_rule_item(struct gpioups_t *upsfdlocal, int newValue) { - int subCount = (upsfdlocal->rules[upsfdlocal->rulesCount-1]) ? upsfdlocal->rules[upsfdlocal->rulesCount-1]->subCount+1 : 1; - int itemSize = subCount*sizeof(upsfdlocal->rules[0]->cRules[0])+sizeof(rulesint); + int subCount = (upsfdlocal->rules[upsfdlocal->rulesCount - 1]) ? upsfdlocal->rules[upsfdlocal->rulesCount - 1]->subCount + 1 : 1; + int itemSize = subCount * sizeof(upsfdlocal->rules[0]->cRules[0]) + sizeof(rulesint); - upsfdlocal->rules[upsfdlocal->rulesCount-1] = xrealloc(upsfdlocal->rules[upsfdlocal->rulesCount-1], itemSize); - upsfdlocal->rules[upsfdlocal->rulesCount-1]->subCount = subCount; - upsfdlocal->rules[upsfdlocal->rulesCount-1]->cRules[subCount-1] = newValue; + upsfdlocal->rules[upsfdlocal->rulesCount - 1] = xrealloc(upsfdlocal->rules[upsfdlocal->rulesCount - 1], itemSize); + upsfdlocal->rules[upsfdlocal->rulesCount - 1]->subCount = subCount; + upsfdlocal->rules[upsfdlocal->rulesCount - 1]->cRules[subCount - 1] = newValue; } /* @@ -131,7 +132,7 @@ void add_rule_item(struct gpioups_t *upsfdlocal, int newValue) { static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ int get_rule_lex(unsigned char *rulesBuff, int *startPos, int *endPos) { - static unsigned char lexType[256]={ + static unsigned char lexType[256] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 00 0x00 */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 16 0x10 */ 0, 0, 0, 0, 0, 0,'&', 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 32 0x20 */ @@ -149,11 +150,11 @@ int get_rule_lex(unsigned char *rulesBuff, int *startPos, int *endPos) { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 224 0xe0 */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 /* 240 0xf0 */ }; - unsigned char lexTypeChr = lexType[rulesBuff[*startPos]]; + unsigned char lexTypeChr = lexType[rulesBuff[*startPos]]; - *endPos = (*startPos)+1; - if(lexTypeChr == 'a' || lexTypeChr == '0') { - for(; lexType[rulesBuff[*endPos]] == lexTypeChr; (*endPos)++); + *endPos = (*startPos) + 1; + if (lexTypeChr == 'a' || lexTypeChr == '0') { + for (; lexType[rulesBuff[*endPos]] == lexTypeChr; (*endPos)++); } return (int)lexTypeChr; } @@ -165,19 +166,19 @@ int get_rule_lex(unsigned char *rulesBuff, int *startPos, int *endPos) { static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { - /* statename=[^]line[&||[line]] */ - char lexBuff[33]; - int startPos = 0, endPos; - int lexType; - int lexStatus = 0; + /* statename = [^]line[&||[line]] */ + char lexBuff[33]; + int startPos = 0, endPos; + int lexType; + int lexStatus = 0; int i, j, k; int tranformationDelta; upsdebugx(4, "rules = [%s]", rulesString); /* state machine to process rules definition */ - while((lexType=get_rule_lex(rulesString, &startPos, &endPos)) > 0 && lexStatus >= 0) { + while ((lexType = get_rule_lex(rulesString, &startPos, &endPos)) > 0 && lexStatus >= 0) { memset(lexBuff, 0, sizeof(lexBuff)); - strncpy(lexBuff, (char *)(rulesString+startPos), endPos-startPos); + strncpy(lexBuff, (char *)(rulesString + startPos), endPos - startPos); upsdebugx(4, "rules start %d, end %d, lexType %d, lex [%s]", startPos, @@ -187,20 +188,20 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { ); switch(lexStatus) { case 0: - if(lexType != 'a') { - lexStatus=-1; + if (lexType != 'a') { + lexStatus = -1; } else { lexStatus = 1; upsfdlocal->rulesCount++; upsfdlocal->rules = xrealloc(upsfdlocal->rules, (size_t)(sizeof(upsfdlocal->rules[0])*upsfdlocal->rulesCount)); - upsfdlocal->rules[upsfdlocal->rulesCount-1] = xcalloc(1, sizeof(rulesint)); - strncpy(upsfdlocal->rules[upsfdlocal->rulesCount-1]->stateName, (char *)(rulesString+startPos), endPos-startPos); - upsfdlocal->rules[upsfdlocal->rulesCount-1]->stateName[endPos-startPos] = 0; + upsfdlocal->rules[upsfdlocal->rulesCount -1 ] = xcalloc(1, sizeof(rulesint)); + strncpy(upsfdlocal->rules[upsfdlocal->rulesCount - 1]->stateName, (char *)(rulesString + startPos), endPos - startPos); + upsfdlocal->rules[upsfdlocal->rulesCount - 1]->stateName[endPos - startPos] = 0; } break; case 1: - if(lexType != '=') { + if (lexType != '=') { lexStatus = -1; } else { lexStatus = 2; @@ -208,35 +209,35 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { break; case 2: - if(lexType == '^') { + if (lexType == '^') { lexStatus = 3; add_rule_item(upsfdlocal, RULES_CMD_NOT); - } else if(lexType == '0') { + } else if (lexType == '0') { lexStatus = 4; - add_rule_item(upsfdlocal, atoi((char *)(rulesString+startPos))); + add_rule_item(upsfdlocal, atoi((char *)(rulesString + startPos))); } else { lexStatus = -1; } break; case 3: - if(lexType != '0') { + if (lexType != '0') { lexStatus = -1; } else { lexStatus = 4; - add_rule_item(upsfdlocal, atoi((char *)(rulesString+startPos))); + add_rule_item(upsfdlocal, atoi((char *)(rulesString + startPos))); } break; case 4: - if(lexType == '&') { + if (lexType == '&') { lexStatus = 2; add_rule_item(upsfdlocal, RULES_CMD_AND); - } else if(lexType == '|') { + } else if (lexType == '|') { lexStatus = 2; add_rule_item(upsfdlocal, RULES_CMD_OR); } - else if(lexType == ';') { + else if (lexType == ';') { lexStatus = 0; } else { lexStatus = -1; @@ -247,22 +248,22 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { lexStatus = -1; break; } - if(lexStatus == -1) + if (lexStatus == -1) fatalx(LOG_ERR, "Line processing rule error at position %d", startPos); startPos = endPos; } - if(lexType == 0 && lexStatus != 0) + if (lexType == 0 && lexStatus != 0) fatalx(LOG_ERR, "Line processing rule error at position %d", startPos); /* debug printout for extracted rules */ upsdebugx(4, "rules count [%d]", upsfdlocal->rulesCount); - for(i = 0; i < upsfdlocal->rulesCount; i++) { + for (i = 0; i < upsfdlocal->rulesCount; i++) { upsdebugx(4, "rule state name [%s], subcount %d", upsfdlocal->rules[i]->stateName, upsfdlocal->rules[i]->subCount ); - for(j = 0; jrules[i]->subCount; j++) { + for (j = 0; jrules[i]->subCount; j++) { upsdebugx(4, "[%s] substate %d [%d]", upsfdlocal->rules[i]->stateName, @@ -275,21 +276,21 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { /* get gpio lines used in rules, find max line number used to check with chip lines count*/ upsfdlocal->upsLinesCount = 0; upsfdlocal->upsMaxLine = 0; - for(i = 0; i < upsfdlocal->rulesCount; i++) { - for(j = 0; j < upsfdlocal->rules[i]->subCount; j++) { - int pinOnList = 0; - for(k = 0; k < upsfdlocal->upsLinesCount && !pinOnList; k++) { - if(upsfdlocal->upsLines[k] == upsfdlocal->rules[i]->cRules[j]) { + for (i = 0; i < upsfdlocal->rulesCount; i++) { + for (j = 0; j < upsfdlocal->rules[i]->subCount; j++) { + int pinOnList = 0; + for (k = 0; k < upsfdlocal->upsLinesCount && !pinOnList; k++) { + if (upsfdlocal->upsLines[k] == upsfdlocal->rules[i]->cRules[j]) { pinOnList = 1; } } - if(!pinOnList) { - if(upsfdlocal->rules[i]->cRules[j] >= 0) { + if (!pinOnList) { + if (upsfdlocal->rules[i]->cRules[j] >= 0) { upsfdlocal->upsLinesCount++; upsfdlocal->upsLines = xrealloc(upsfdlocal->upsLines, sizeof(upsfdlocal->upsLines[0])*upsfdlocal->upsLinesCount); - upsfdlocal->upsLines[upsfdlocal->upsLinesCount-1] = upsfdlocal->rules[i]->cRules[j]; - if(upsfdlocal->upsLines[upsfdlocal->upsLinesCount-1] > upsfdlocal->upsMaxLine) { - upsfdlocal->upsMaxLine = upsfdlocal->upsLines[upsfdlocal->upsLinesCount-1]; + upsfdlocal->upsLines[upsfdlocal->upsLinesCount - 1] = upsfdlocal->rules[i]->cRules[j]; + if (upsfdlocal->upsLines[upsfdlocal->upsLinesCount - 1] > upsfdlocal->upsMaxLine) { + upsfdlocal->upsMaxLine = upsfdlocal->upsLines[upsfdlocal->upsLinesCount - 1]; } } } @@ -297,23 +298,23 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { } upsdebugx(4, "UPS line count = %d", upsfdlocal->upsLinesCount); - for(i = 0; i < upsfdlocal->upsLinesCount; i++) { + for (i = 0; i < upsfdlocal->upsLinesCount; i++) { upsdebugx(4, "UPS line%d number %d", i, upsfdlocal->upsLines[i]); } /* transform lines to indexes for easier state calculation */ tranformationDelta = upsfdlocal->upsMaxLine - RULES_CMD_LAST + 1; - for(i = 0; i < upsfdlocal->rulesCount; i++) { - for(j = 0; j < upsfdlocal->rules[i]->subCount; j++) { - if(upsfdlocal->rules[i]->cRules[j] >= 0) { + for (i = 0; i < upsfdlocal->rulesCount; i++) { + for (j = 0; j < upsfdlocal->rules[i]->subCount; j++) { + if (upsfdlocal->rules[i]->cRules[j] >= 0) { upsfdlocal->rules[i]->cRules[j] -= tranformationDelta; } } } - for(k = 0; k < upsfdlocal->upsLinesCount; k++) { - for(i = 0; i < upsfdlocal->rulesCount; i++) { - for(j = 0; j < upsfdlocal->rules[i]->subCount; j++) { - if((upsfdlocal->rules[i]->cRules[j] + tranformationDelta) == upsfdlocal->upsLines[k]) { + for (k = 0; k < upsfdlocal->upsLinesCount; k++) { + for (i = 0; i < upsfdlocal->rulesCount; i++) { + for (j = 0; j < upsfdlocal->rules[i]->subCount; j++) { + if ((upsfdlocal->rules[i]->cRules[j] + tranformationDelta) == upsfdlocal->upsLines[k]) { upsfdlocal->rules[i]->cRules[j] = k; } } @@ -322,13 +323,13 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { /* debug printout of transformed lines numbers */ upsdebugx(4, "rules count [%d] translated", upsfdlocal->rulesCount); - for(i = 0; i < upsfdlocal->rulesCount; i++) { + for (i = 0; i < upsfdlocal->rulesCount; i++) { upsdebugx(4, "rule state name [%s], subcount %d translated", upsfdlocal->rules[i]->stateName, upsfdlocal->rules[i]->subCount ); - for(j = 0; j < upsfdlocal->rules[i]->subCount; j++) { + for (j = 0; j < upsfdlocal->rules[i]->subCount; j++) { upsdebugx(4, "[%s] substate %d [%d]", upsfdlocal->rules[i]->stateName, j, @@ -345,11 +346,12 @@ void get_ups_rules(struct gpioups_t *upsfdlocal, unsigned char *rulesString) { static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ int calc_rule_states(int upsLinesStates[], int cRules[], int subCount, int sIndex) { - int ruleVal = 0; - int iopStart = sIndex; - int rs; - if(iopStart < subCount) { /* calculate left side */ - if(cRules[iopStart] >= 0) { + int ruleVal = 0; + int iopStart = sIndex; + int rs; + + if (iopStart < subCount) { /* calculate left side */ + if (cRules[iopStart] >= 0) { ruleVal = upsLinesStates[cRules[iopStart]]; } else { iopStart++; @@ -357,13 +359,14 @@ int calc_rule_states(int upsLinesStates[], int cRules[], int subCount, int sInde } iopStart++; } - for(; iopStart < subCount; iopStart++) { /* right side calculation */ - if(cRules[iopStart] == RULES_CMD_OR) { - ruleVal = ruleVal || calc_rule_states(upsLinesStates, cRules, subCount, iopStart+1); + + for (; iopStart < subCount; iopStart++) { /* right side calculation */ + if (cRules[iopStart] == RULES_CMD_OR) { + ruleVal = ruleVal || calc_rule_states(upsLinesStates, cRules, subCount, iopStart + 1); break; } else { iopStart++; - if(cRules[iopStart] == RULES_CMD_NOT) { + if (cRules[iopStart] == RULES_CMD_NOT) { iopStart++; rs = !upsLinesStates[cRules[iopStart]]; } else { @@ -384,39 +387,39 @@ int calc_rule_states(int upsLinesStates[], int cRules[], int subCount, int sInde static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ void update_ups_states(struct gpioups_t *gpioupsfdlocal) { - int batLow = 0; - int bypass = 0; - int chargerStatusSet = 0; - int ruleNo; + int batLow = 0; + int bypass = 0; + int chargerStatusSet = 0; + int ruleNo; status_init(); - for(ruleNo = 0; ruleNo < gpioupsfdlocal->rulesCount; ruleNo++) { + for (ruleNo = 0; ruleNo < gpioupsfdlocal->rulesCount; ruleNo++) { gpioupsfdlocal->rules[ruleNo]->currVal = calc_rule_states( gpioupsfdlocal->upsLinesStates, gpioupsfdlocal->rules[ruleNo]->cRules, gpioupsfdlocal->rules[ruleNo]->subCount, 0 ); - if(gpioupsfdlocal->rules[ruleNo]->currVal) { + if (gpioupsfdlocal->rules[ruleNo]->currVal) { status_set(gpioupsfdlocal->rules[ruleNo]->stateName); - if(!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "CHRG")) { + if (!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "CHRG")) { dstate_setinfo("battery.charger.status", "%s", "charging"); chargerStatusSet++; } - if(!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "DISCHRG")) { + if (!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "DISCHRG")) { dstate_setinfo("battery.charger.status", "%s", "discharging"); chargerStatusSet++; } - if(!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "LB")) { + if (!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "LB")) { batLow = 1; } - if(!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "BYPASS")) { + if (!strcmp(gpioupsfdlocal->rules[ruleNo]->stateName, "BYPASS")) { bypass = 1; } } - if(gpioupsfdlocal->aInfoAvailable && + if (gpioupsfdlocal->aInfoAvailable && gpioupsfdlocal->rules[ruleNo]->archVal != gpioupsfdlocal->rules[ruleNo]->currVal) { upslogx(LOG_WARNING, "UPS state [%s] changed to %d", gpioupsfdlocal->rules[ruleNo]->stateName, @@ -426,19 +429,19 @@ void update_ups_states(struct gpioups_t *gpioupsfdlocal) { gpioupsfdlocal->rules[ruleNo]->archVal = gpioupsfdlocal->rules[ruleNo]->currVal; } - if(chargerStatusSet <= 0) { + if (chargerStatusSet <= 0) { dstate_delinfo("battery.charger.status"); } - if(dstate_getinfo("battery.charge.low")!=NULL) { - if(batLow) { + if (dstate_getinfo("battery.charge.low") != NULL) { + if (batLow) { dstate_setinfo("battery.charge", "%s", dstate_getinfo("battery.charge.low")); } else { dstate_setinfo("battery.charge", "%s", "100"); } } - if(bypass) { + if (bypass) { dstate_delinfo("battery.charge"); } @@ -449,10 +452,10 @@ void update_ups_states(struct gpioups_t *gpioupsfdlocal) { void upsdrv_initinfo(void) { - if(testvar("mfr")) { + if (testvar("mfr")) { dstate_setinfo("device.mfr", "%s", getval("mfr")); } - if(testvar("model")) { + if (testvar("model")) { dstate_setinfo("device.model", "%s", getval("model")); } } @@ -495,9 +498,9 @@ void upsdrv_makevartable(void) void upsdrv_initups(void) { /* prepare rules and allocate related structures */ - gpioupsfd=generic_gpio_open(device_path); + gpioupsfd = generic_gpio_open(device_path); /* open GPIO chip and check pin consistence */ - if(gpioupsfd) { + if (gpioupsfd) { gpio_open(gpioupsfd); } } From 63c02b796a0e142608db3e353e007212fb821466 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:53:34 +0100 Subject: [PATCH 14/22] drivers/generic_gpio_common.c: upsdrv_cleanup(): only free resources once Check if (gpioupsfd!=NULL) and nullify it afterwards. Should not be a problem in the real driver, but confused the repetitive use of this code in unit test case. Signed-off-by: Jim Klimov --- drivers/generic_gpio_common.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/generic_gpio_common.c b/drivers/generic_gpio_common.c index 059af237c9..93b240c709 100644 --- a/drivers/generic_gpio_common.c +++ b/drivers/generic_gpio_common.c @@ -104,6 +104,7 @@ void generic_gpio_close(struct gpioups_t *gpioupsfdlocal) { } } free(gpioupsfdlocal); + /* caller is encouraged to do the same with their copy: */ gpioupsfdlocal = NULL; } } @@ -507,8 +508,11 @@ void upsdrv_initups(void) void upsdrv_cleanup(void) { - /* release gpio library resources */ - gpio_close(gpioupsfd); - /* release related generic resources */ - generic_gpio_close(gpioupsfd); + if (gpioupsfd) { + /* release gpio library resources */ + gpio_close(gpioupsfd); + /* release related generic resources */ + generic_gpio_close(gpioupsfd); + gpioupsfd = NULL; + } } From ab00ea946b99b92fb2fc31b5d031f4dd94e494f3 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 16:55:17 +0100 Subject: [PATCH 15/22] tests/generic_gpio_utest.c: do upsdrv_cleanup() now that it is safe to occasionally repeat the attempt Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index 6ff14b0704..33befe06c7 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -336,7 +336,7 @@ int main(int argc, char **argv) { failed = expecting_failure; if(jmp_result) { /* test case exiting */ if(expecting_failure) failed=0; - /* upsdrv_cleanup(); */ + upsdrv_cleanup(); } else { if(expecting_failure) failed=1; device_path = chipNameLocal; @@ -390,6 +390,9 @@ int main(int argc, char **argv) { done = 1; dstate_free(); + /* Should be safe if we happen to run this twice for + * generic_gpio_common.c, it only frees driver variables once */ + upsdrv_cleanup(); /* Return 0 (exit-code OK, boolean false) if no tests failed and some ran */ if ( (cases_failed == 0) && (cases_passed > 0) ) From 93be140bb36aa6324e162b059fde56a207eb4046 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 17:11:01 +0100 Subject: [PATCH 16/22] drivers/generic_gpio_common.c: generic_gpio_close(): free(gpioupsfdlocal->rules) itself too, not just entries in the list Signed-off-by: Jim Klimov --- drivers/generic_gpio_common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/generic_gpio_common.c b/drivers/generic_gpio_common.c index 93b240c709..9749121ac3 100644 --- a/drivers/generic_gpio_common.c +++ b/drivers/generic_gpio_common.c @@ -102,6 +102,7 @@ void generic_gpio_close(struct gpioups_t *gpioupsfdlocal) { for (i = 0; i < gpioupsfdlocal->rulesCount; i++) { free(gpioupsfdlocal->rules[i]); } + free(gpioupsfdlocal->rules); } free(gpioupsfdlocal); /* caller is encouraged to do the same with their copy: */ From 42d8f3cac3615e67d589e3e9f35d59da840a29dc Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 18:14:07 +0100 Subject: [PATCH 17/22] drivers/generic_gpio_common.{c,h}, tests/generic_gpio_utest.c: refactor generic_gpio_close() to take a pointer to pointer and nullify it Signed-off-by: Jim Klimov --- drivers/generic_gpio_common.c | 30 ++++++++++++++---------------- drivers/generic_gpio_common.h | 2 +- tests/generic_gpio_utest.c | 8 ++++---- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/drivers/generic_gpio_common.c b/drivers/generic_gpio_common.c index 9749121ac3..ef132e8d4a 100644 --- a/drivers/generic_gpio_common.c +++ b/drivers/generic_gpio_common.c @@ -35,7 +35,7 @@ struct gpioups_t *generic_gpio_open(const char *chipName); #ifndef DRIVERS_MAIN_WITHOUT_MAIN static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ -void generic_gpio_close(struct gpioups_t *gpioupsfdlocal); +void generic_gpio_close(struct gpioups_t **gpioupsfdptr); #ifndef DRIVERS_MAIN_WITHOUT_MAIN static @@ -89,24 +89,23 @@ struct gpioups_t *generic_gpio_open(const char *chipName) { #ifndef DRIVERS_MAIN_WITHOUT_MAIN static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ -void generic_gpio_close(struct gpioups_t *gpioupsfdlocal) { - if (gpioupsfdlocal) { - if (gpioupsfdlocal->upsLines) { - free(gpioupsfdlocal->upsLines); +void generic_gpio_close(struct gpioups_t **gpioupsfdptr) { + if (gpioupsfdptr && *gpioupsfdptr) { + if ((*gpioupsfdptr)->upsLines) { + free((*gpioupsfdptr)->upsLines); } - if (gpioupsfdlocal->upsLinesStates) { - free(gpioupsfdlocal->upsLinesStates); + if ((*gpioupsfdptr)->upsLinesStates) { + free((*gpioupsfdptr)->upsLinesStates); } - if (gpioupsfdlocal->rules) { + if ((*gpioupsfdptr)->rules) { int i; - for (i = 0; i < gpioupsfdlocal->rulesCount; i++) { - free(gpioupsfdlocal->rules[i]); + for (i = 0; i < (*gpioupsfdptr)->rulesCount; i++) { + free((*gpioupsfdptr)->rules[i]); } - free(gpioupsfdlocal->rules); + free((*gpioupsfdptr)->rules); } - free(gpioupsfdlocal); - /* caller is encouraged to do the same with their copy: */ - gpioupsfdlocal = NULL; + free(*gpioupsfdptr); + *gpioupsfdptr = NULL; } } @@ -513,7 +512,6 @@ void upsdrv_cleanup(void) /* release gpio library resources */ gpio_close(gpioupsfd); /* release related generic resources */ - generic_gpio_close(gpioupsfd); - gpioupsfd = NULL; + generic_gpio_close(&gpioupsfd); } } diff --git a/drivers/generic_gpio_common.h b/drivers/generic_gpio_common.h index 5589fc7ec7..7e92a8965e 100644 --- a/drivers/generic_gpio_common.h +++ b/drivers/generic_gpio_common.h @@ -75,7 +75,7 @@ void gpio_close(struct gpioups_t *gpioupsfd); # ifdef DRIVERS_MAIN_WITHOUT_MAIN /* Methods externalized for unit-tests, otherwise private to this module */ struct gpioups_t *generic_gpio_open(const char *chipName); -void generic_gpio_close(struct gpioups_t *gpioupsfd); +void generic_gpio_close(struct gpioups_t **gpioupsfdptr); void get_ups_rules(struct gpioups_t *upsfd, unsigned char *rulesString); void add_rule_item(struct gpioups_t *upsfd, int newValue); int get_rule_lex(unsigned char *rulesBuff, int *startPos, int *endPos); diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index 33befe06c7..bc8a6b889c 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) { if(fEof!=EOF) { if(!strcmp(testType, "rules")) { struct gpioups_t *upsfdtest = xcalloc(1, sizeof(*upsfdtest)); - /* NOTE: here and below, freed by generic_gpio_close(upsfdtest) */ + /* NOTE: here and below, freed by generic_gpio_close(&upsfdtest) */ jmp_result = setjmp(env_buffer); if(jmp_result) { /* test case exiting */ printf("%s %s test rule %u [%s]\n", pass_fail[get_test_status(upsfdtest, 1)], testType, i, rules); @@ -219,7 +219,7 @@ int main(int argc, char **argv) { get_ups_rules(upsfdtest, (unsigned char *)rules); printf("%s %s test rule %u [%s]\n", pass_fail[get_test_status(upsfdtest, 0)], testType, i, rules); } - generic_gpio_close(upsfdtest); + generic_gpio_close(&upsfdtest); } if(!strcmp(testType, "states")) { int expectedStateValue; @@ -248,7 +248,7 @@ int main(int argc, char **argv) { cases_failed++; } } - generic_gpio_close(upsfdtest); + generic_gpio_close(&upsfdtest); } if(!strcmp(testType, "update")) { char upsStatus[256]; @@ -292,7 +292,7 @@ int main(int argc, char **argv) { if( strcmp(chargeLow,".") && strcmp(charge,".") && (!currCharge || strcmp(currCharge, charge))) failed=1; if(!strcmp(chargeLow,".") && !strcmp(charge,".") && currCharge!=NULL) failed=1; } - generic_gpio_close(upsfdtest); + generic_gpio_close(&upsfdtest); printf("%s %s test rule %u [%s] ([%s] %s %s (%s)) ([%s] %s %s)\n", pass_fail[failed], testType, i, rules, upsStatus, chargeStatus, charge, chargeLow, From bf59f10950246f1dd8ea22e58d501502c7bd499d Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 18:17:03 +0100 Subject: [PATCH 18/22] tests/generic_gpio_utest.c: Be sure the internal gpioupsfd in driver code is freed before we (re?)init it in test Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index bc8a6b889c..a8afdbd203 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -334,9 +334,10 @@ int main(int argc, char **argv) { #endif jmp_result = setjmp(env_buffer); failed = expecting_failure; + /* Be sure the internal gpioupsfd in driver code is freed before we init it below */ + upsdrv_cleanup(); if(jmp_result) { /* test case exiting */ if(expecting_failure) failed=0; - upsdrv_cleanup(); } else { if(expecting_failure) failed=1; device_path = chipNameLocal; From 29053707117a919f099c4e3ada0222ec891aeac3 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 18:39:48 +0100 Subject: [PATCH 19/22] tests/generic_gpio_utest.c: fix inverted strcmp() result treatment Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index a8afdbd203..69889aebda 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -101,7 +101,7 @@ int get_test_status(struct gpioups_t *result, int on_fail_path) { for (i=0; irulesCount; i++) { fEof=fscanf(testData, "%s", stateName); - if(!strcmp(result->rules[i]->stateName,stateName)) { + if(strcmp(result->rules[i]->stateName,stateName)) { cases_failed++; printf("expecting stateName %s, got %s for rule %d\n", stateName, result->rules[i]->stateName, i); return 1; From dc052aea2d232b2e1a606c0ed532255b0731b859 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 19:40:24 +0100 Subject: [PATCH 20/22] drivers/generic_gpio_common.c: generic_gpio_open(): reshuffle to optimize * Less lookups to testval/getval * Do not xalloc and fatalx if got no rules (avoid leak) Signed-off-by: Jim Klimov --- drivers/generic_gpio_common.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/generic_gpio_common.c b/drivers/generic_gpio_common.c index ef132e8d4a..1d99db421a 100644 --- a/drivers/generic_gpio_common.c +++ b/drivers/generic_gpio_common.c @@ -69,15 +69,18 @@ void update_ups_states(struct gpioups_t *gpioupsfdlocal); static #endif /* DRIVERS_MAIN_WITHOUT_MAIN */ struct gpioups_t *generic_gpio_open(const char *chipName) { - struct gpioups_t *upsfdlocal = xcalloc(1, sizeof(*upsfdlocal)); + char *rules = getval("rules"); + struct gpioups_t *upsfdlocal = NULL; + + if (!rules) /* rules is required configuration parameter */ + fatalx(EXIT_FAILURE, "UPS status calculation rules not specified"); + + upsfdlocal = xcalloc(1, sizeof(*upsfdlocal)); upsfdlocal->runOptions = 0; /* don't use ROPT_REQRES and ROPT_EVMODE yet */ upsfdlocal->chipName = chipName; - if (!testvar("rules")) /* rules is required configuration parameter */ - fatalx(EXIT_FAILURE, "UPS status calculation rules not specified"); - - get_ups_rules(upsfdlocal, (unsigned char *)getval("rules")); + get_ups_rules(upsfdlocal, (unsigned char *)rules); upsfdlocal->upsLinesStates = xcalloc(upsfdlocal->upsLinesCount, sizeof(int)); return upsfdlocal; From 6bc1e16cae6a512d795c2ffdda4b8760693bbb5b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 19:42:11 +0100 Subject: [PATCH 21/22] Revert "tests/generic_gpio_utest.c: Be sure the internal gpioupsfd in driver code is freed before we (re?)init it in test" This reverts commit bf59f10950246f1dd8ea22e58d501502c7bd499d. Signed-off-by: Jim Klimov --- tests/generic_gpio_utest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/generic_gpio_utest.c b/tests/generic_gpio_utest.c index 69889aebda..3c171672df 100644 --- a/tests/generic_gpio_utest.c +++ b/tests/generic_gpio_utest.c @@ -334,10 +334,9 @@ int main(int argc, char **argv) { #endif jmp_result = setjmp(env_buffer); failed = expecting_failure; - /* Be sure the internal gpioupsfd in driver code is freed before we init it below */ - upsdrv_cleanup(); if(jmp_result) { /* test case exiting */ if(expecting_failure) failed=0; + upsdrv_cleanup(); } else { if(expecting_failure) failed=1; device_path = chipNameLocal; From 6384e99e2f3e603846f540e2037651bf4d6433d2 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 25 Jan 2025 20:13:51 +0100 Subject: [PATCH 22/22] scripts/valgrind/valgrind.sh.in: revise detection of VALGRIND and LIBTOOL values Signed-off-by: Jim Klimov --- scripts/valgrind/valgrind.sh.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/valgrind/valgrind.sh.in b/scripts/valgrind/valgrind.sh.in index 4719d6646a..966c29f751 100755 --- a/scripts/valgrind/valgrind.sh.in +++ b/scripts/valgrind/valgrind.sh.in @@ -8,12 +8,13 @@ SCRIPTDIR="`dirname "$0"`" SCRIPTDIR="`cd "$SCRIPTDIR" && pwd`" LTEXEC="" +[ -n "${LIBTOOL-}" ] || LIBTOOL="`command -v glibtool`" 2>/dev/null [ -n "${LIBTOOL-}" ] || LIBTOOL="`command -v libtool`" 2>/dev/null [ -z "${LIBTOOL}" ] || LTEXEC="${LIBTOOL} --mode=execute " # Allow to run un-parsed "sh ${top_srcdir}/scripts/valgrind/valgrind.sh.in" # with reasonable success: -[ -n "${VALGRIND-}" ] || case '@VALGRIND@' in [@]*) ;; *) VALGRIND='@VALGRIND@' ;; esac +[ -n "${VALGRIND-}" ] || case '@VALGRIND@' in [@]*|none) ;; *) VALGRIND='@VALGRIND@' ;; esac [ -n "${VALGRIND-}" ] || VALGRIND="valgrind" # TODO: Also wrap tool=callgrind e.g. via $0 symlink name?