© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
All good. You're very careful and detailed with all that you do and that is a very good signal. Anyone reviewing your cases can see you clearly focus in minor details. I particularly like that style and usually read everything written in cases, even if big comments and diccussions.
Nevertheless, for the SRU template, that might not be true for the SRU reviewers. Not that they may dislike reading complete analysis.. but sometimes they lack time. I'll give an example so things are more clear:
-------
"""
[Regression Potential]
Since the affected code deals with executing a subprocess, reading its output through a pipe, and then relying on wait(2) to determine whether the subprocess exited correctly; and considering that this code is written in C without the help of features like RAII and with the use of goto statements, we are not able to disconsider the chances of making a mistake and forgetting to free a resource or to properly read/write from/to pipes, for example. This is, after all, the reason the bug happened in the first place.
Having said that, openssh contains extensive tests and the code is well organized and relatively easy to follow. Upon close inspection, there doesn't seem to be an evident problem with the backported fixes.
As usual when dealing with a somewhat older distribution, there is always the possibility of encountering problems because we will be recompiling openssh using the most recent versions of its build dependencies.
"""
I usually use the bullets (from the template) and list one by one:
----
[Regression Potential]
* code forks and wait(2)s for clean exit, could lead to fd or mem leaks
* upstream code has tests, ran testcase couldn't find leaks
* ran valgrind before/after my fix, no leaks (to mitigate the potential)
----
3 bullets and SRU team knows what to expect. IF reviewer thinks is worth investigating further, he/she might look into bug details/comments.
-------
Anyway, apart from my suggestions, your work is very solid.
+1 from me for this merge.