Univention Bugzilla – Bug 49277
Remove dead UDN code
Last modified: 2021-10-13 10:42:52 CEST
+++ This bug was initially created as a clone of Bug #41687 +++ 1. The code uses pthread_*() while the code is single-threaded. Remove! 2. sem.c implements semaphores, which are un-needed. 3. debian/univention-ldap-notifier.postinst is unused 8. Unused notify_entry_reverse() 9. Commented out code in notify_replog_change_callback() - maybe remove "replog" code altogether?
(In reply to Philipp Hahn from comment #0) > 1. The code uses pthread_*() while the code is single-threaded. Remove! [phahn/49277-udn-cleanup] 19c51f0058 Bug #49277 udn.1: Remove pthread code > 2. sem.c implements semaphores, which are un-needed. > 3. debian/univention-ldap-notifier.postinst is unused Already removed release-4.2-0~2531 > 8. Unused notify_entry_reverse() > 19. Dead code: [phahn/49277-udn-cleanup] 6973a8505a Bug #49277 udn.8: Remove dead code > 9. Commented out code in notify_replog_change_callback() - maybe remove > "replog" code altogether? [phahn/49277-udn-cleanup] c3f3e3b342 Bug #49277 udn.9: Remove dead signal code [phahn/49277-udn-cleanup] 7313f17cb2 Bug #49277 udn.9: Remove old replog code > 20. Dead declarations: [phahn/49277-udn-cleanup] 1797d84200 Bug #49277 udn.20: Remove forward declarations [phahn/49277-udn-cleanup] 1c06e070e6 Bug #49277 udn.20: Remove unimplemented functions > 27. Remove unused variable "rc" [phahn/49277-udn-cleanup] cec28859e7 Bug #49277 udn.27: Remove dead variable > 28. Unused tmp_buf [phahn/49277-udn-cleanup] 88b8deebe8 Bug #49277 udn.28: Remove unused tmp_buf > 29. Remove unused instance variables [phahn/49277-udn-cleanup] 72fce01ca3 Bug #49277 udn.29: Remove dead instance variables <http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-0/view/Branch%20Tests/job/branch%20test%20base%20singlemaster/37/console> [phahn/49277-udn-cleanup] ec47154058 Bug #49277 UDN: Remove unused and deprecated code deb [trusted=yes] http://10.200.18.180/debian/ phahn49277-udn-cleanup main univention-directory-notifier_13.0.1-14A~~20190411.154825.ec471540587816512ff26fd40f1a587148e1b16c_amd64.deb PS: <https://git.knut.univention.de/univention/ucs/commits/phahn/41687-udn-code> contains several more cleanups I would like to apply, but they do some code moving and re-writing, so I keep them for the next round.
<https://git.knut.univention.de/univention/ucs/commits/phahn/49277-udn-cleanup?utf8=%E2%9C%93&search=Bug+%2349277>
Created attachment 9967 [details] qa-feedback.patch Ok, just to be fair, this is a bit more than we already agreed upon: Philipp Hahn (11): Bug #49277 udn.1: Remove pthread code Bug #49277 udn.2: Remove semaphore code Bug #49277 udn.8: Remove dead code Bug #49277 udn.9: Remove dead signal code Bug #49277 udn.9: Remove old replog code Bug #49277 udn.20: Remove forward declarations Bug #49277 udn.20: Remove unimplemented functions Bug #49277 udn.27: Remove dead variable Bug #49277 udn.28: Remove unused tmp_buf Bug #49277 udn.29: Remove dead instance variables Bug #49277 UDN: Remove unused and deprecated code Anyway: * Code review ok, proposal for minor changes attached * Btw: I'm unsure about the directory mode for /var/lib/univention-ldap/notify The postinst always has set it to 700, but on my (updated from 4.1) has 755, also one of our internal servers. Couldn't find a quick explanation for this. * The branch test (ldap) worked with the default singlemaster.cfg * Unfortunately the branch test didn't work with branch-tests/base/master-slave.cfg: http://jenkins.knut.univention.de:8080/job/UCS%20Branch%20Test/109/console -- from the log file: ============================================================================== => Execute: . utils.sh && assert_join on [master] at 2019-04-11T18:03:02.617915 [master] 2019-04-11T18:03:02.658648 + assert_join [master] 2019-04-11T18:03:02.659288 ++ seq 1 3 [master] 2019-04-11T18:03:02.663330 + for i in $(seq 1 3) [master] 2019-04-11T18:03:02.663330 + univention-check-join-status [master] 2019-04-11T18:03:02.894080 Error: /etc/machine.secret not found ============================================================================== * Whatever, the package compile works, I also did it on my VM and did a functional test of the notifier So, please check my patch proposal, merge your branch to 4.4-0, build and update the yaml. Then we'll see the nightly test results for confirmation. Should be fine.
(In reply to Arvid Requate from comment #3) > Created attachment 9967 [details] > qa-feedback.patch > > Ok, just to be fair, this is a bit more than we already agreed upon: 1st of all: thank you for doing the review. > * Btw: I'm unsure about the directory mode for > /var/lib/univention-ldap/notify > The postinst always has set it to 700, but on my (updated from 4.1) has > 755, > also one of our internal servers. Couldn't find a quick explanation for > this. That directory is listed in debian/univention-directory-notifier.dirs, so it get created by dpkg when unpacking the binary package. The mode defaults to 0o755, so that "mkdir -p" and "chmod" in the .postinst never created the directory and changed the permission as it already existed. Indeed using "install -d" would have changed the behavior as it would have changed to permissions. Instead I changed the code to not use "mkdir" in .postinst at all and moved the directories all to .dirs. [phahn/49277-udn-cleanup] 5433f2bb26 fixup! Bug #49277 udn.9: Remove old replog code .../debian/univention-directory-notifier.dirs | 2 ++ .../debian/univention-directory-notifier.postinst | 10 ---------- 2 files changed, 2 insertions(+), 10 deletions(-) > * Unfortunately the branch test didn't work with > branch-tests/base/master-slave.cfg: > http://jenkins.knut.univention.de:8080/job/UCS%20Branch%20Test/109/console No idea so far why this change would trigger that. I will look further. > So, please check my patch proposal, merge your branch to 4.4-0, build and > update the yaml. Then we'll see the nightly test results for confirmation. > Should be fine. I did not do any white-space clean-up on purpose not not hide some changed in it. It it is okay with you I would like to run "clang-format" on it once as the code contains lots more of strange white-space use. See <https://hutten.knut.univention.de/mediawiki/index.php/Code-Richtlinien#C> for my proposed style.
[phahn/49277-udn-cleanup] 30419345c0 fixup! Bug #49277 udn.29: Remove dead instance variables management/univention-directory-notifier/src/network.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
[4.4-0] 058ba8c6fe Bug #49277 UDN: Remove unused and deprecated code [4.4-0] 4317870541 Bug #49277 UDN: Remove unused and deprecated code [4.4-0] b68e4c9e65 Bug #49277 udn.29: Remove dead instance variables [4.4-0] 7e509d445f Bug #49277 udn.28: Remove unused tmp_buf [4.4-0] a58496102e Bug #49277 udn.27: Remove dead variable [4.4-0] 20f2d39b57 Bug #49277 udn.20: Remove unimplemented functions [4.4-0] d7b620fdf3 Bug #49277 udn.20: Remove forward declarations [4.4-0] 77c37e4a89 Bug #49277 udn.9: Remove old replog code I squashed in one more line to also remove ONLY_NOTIFY from notify.c [4.4-0] f03e1b1017 Bug #49277 udn.9: Remove dead signal code [4.4-0] 87a8514bd6 Bug #49277 udn.8: Remove dead code [4.4-0] b547eff88e Bug #49277 udn.2: Remove semaphore code [4.4-0] 2e62b70dcd Bug #49277 udn.1: Remove pthread code Package: univention-directory-notifier Version: 13.0.1-14A~4.4.0.201904230742 Branch: ucs_4.4-0 Scope: errata4.4-0 [4.4-0] 9dfbc4f630 Bug #49277: univention-directory-notifier 13.0.1-14A~4.4.0.201904230742 doc/errata/staging/univention-directory-notifier.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Verified: * Code review (merged commits) * Functional test * Advisory
<http://errata.software-univention.de/ucs/4.4/54.html>