Bug 49277 - Remove dead UDN code
Remove dead UDN code
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Notifier (univention-directory-notifier)
UCS 4.4
Other Linux
: P5 normal (vote)
: UCS 4.4-0-errata
Assigned To: Philipp Hahn
Arvid Requate
:
Depends on:
Blocks: 41687
  Show dependency treegraph
 
Reported: 2019-04-11 13:34 CEST by Philipp Hahn
Modified: 2021-10-13 10:42 CEST (History)
3 users (show)

See Also:
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:
hahn: Patch_Available+


Attachments
qa-feedback.patch (2.90 KB, patch)
2019-04-11 18:58 CEST, Arvid Requate
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philipp Hahn univentionstaff 2019-04-11 13:34:45 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?
Comment 1 Philipp Hahn univentionstaff 2019-04-11 15:54:16 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.
Comment 3 Arvid Requate univentionstaff 2019-04-11 18:58:50 CEST
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.
Comment 4 Philipp Hahn univentionstaff 2019-04-18 20:34:25 CEST
(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.
Comment 5 Philipp Hahn univentionstaff 2019-04-18 20:38:59 CEST
[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(-)
Comment 6 Philipp Hahn univentionstaff 2019-04-23 07:46:15 CEST
[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(-)
Comment 7 Arvid Requate univentionstaff 2019-04-23 19:02:53 CEST
Verified:
* Code review (merged commits)
* Functional test
* Advisory
Comment 8 Arvid Requate univentionstaff 2019-04-24 13:13:05 CEST
<http://errata.software-univention.de/ucs/4.4/54.html>