kdenetwork/ksirc

[George Staikos]

On Thursday 19 December 2002 12:33, Andrew Stanley-Jones wrote:
> This was CC'ed to core devel a few days ago, so I thought I might as well
> follow up.  While my first patch was wrong, but  this brings up a very old
> topic, and maybe it could refresh some memories.
>
> The change to the code (strncpy->strlcpy) was simple and even though it
> "should not" do anything bad, broke the entire program making it useless.*
> Since the change was made by someone who did not attempt to understand the
> code, and then never tested the problem wasn't found till much later.  I
> fixed head since I ran accross it.
>
> Yesterday while on irc on #kde-devel a little message poped up, "anyone
> tried kSirc in 3.1 branch recently?"  Client was completly broken.  Again a
> simple fix, and we got it good and early so no harm done.
>
> But it really bugs me when changes are done and not tested.  I understand
> that when changing lots of code all over the place it would be very time
> consuming, but especially in stable branches how else do you know it was
> good?  If you don't understand the code/or are unwilling to test it maybe
> email the maintainer?

   I think the root of the problem lies elsewhere.  If you look at the number 
of problems we found (and the number of problems that probably still exist), 
you will realise that four people, two working part time for free, cannot 
possibly test all of that code.  Emailing maintainers is easy, if they even 
exist, but often leads to dead ends, no response, or very long response 
times.  It is the maintainer's responsibility to monitor his code for changes 
too.  If no-one is monitoring code in CVS, then it is effectively 
unmaintained, despite what any text files might say.

   The fact that these changes had to be made at all in CVS is yet another 
problem.  If there weren't so many unfortunate problems with the code, the 
changes never would have been made in the first place.  In this case, it 
happened to not be a bug at all, but what is 1 false hit in a sea of 
hundreds?  Statistically unnoticeable.

   Finally, I think there were problems with this code afterall.  str*() are 
used for manipulating null-terminated strings.  Yes you can do other things 
with them, but you can virtually write any app using only system() too.  This 
doesn't make it correct.  mem*() are much better alternatives when you are 
dealing with non-null terminated data.  The changes were of course made and 
this will help in the future I'm sure.  A good alternative would have been a 
comment which described that even though str*() were used, the null byte is 
being disregarded if it exists.  Unusual code is perhaps most deserving of 
comments.

   Of course this is no excuse to be careless with these fixes, but 
nonetheless they have to be done, and they have to be done to the best of our 
abilities within our constraints.

-- 

George Staikos