Univention Bugzilla – Full Text Bug Listing |
Summary: | Remove dead UDN code | ||
---|---|---|---|
Product: | UCS | Reporter: | Philipp Hahn <hahn> |
Component: | Notifier (univention-directory-notifier) | Assignee: | Philipp Hahn <hahn> |
Status: | CLOSED FIXED | QA Contact: | Arvid Requate <requate> |
Severity: | normal | ||
Priority: | P5 | CC: | gohmann, requate, schwardt |
Version: | UCS 4.4 | Flags: | hahn:
Patch_Available+
|
Target Milestone: | UCS 4.4-0-errata | ||
Hardware: | Other | ||
OS: | Linux | ||
What kind of report is it?: | Development Internal | What type of bug is this?: | --- |
Who will be affected by this bug?: | --- | How will those affected feel about the bug?: | --- |
User Pain: | Enterprise Customer affected?: | ||
School Customer affected?: | ISV affected?: | ||
Waiting Support: | Flags outvoted (downgraded) after PO Review: | ||
Ticket number: | Bug group (optional): | Cleanup | |
Max CVSS v3 score: | |||
Bug Depends on: | |||
Bug Blocks: | 41687 | ||
Attachments: | qa-feedback.patch |
Description
Philipp Hahn
2019-04-11 13:34:45 CEST
(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. 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 |