[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: KMD5
From:       Dawit Alemayehu <adawit () kde ! org>
Date:       2001-11-17 7:49:15
[Download RAW message or body]

Hi Dirk,

> The things in particular:
>
> - KMD5 was not able to md5 a set of binary data. Thats because it assumed
>   that it can call strlen() on it to get the length. Thats broken.

Point taken for the ctor that did this blindly, but alas this only happened in one ctor!  
The other place where this is used is in the update call which provides a parameter 
for specifying the and you kept that :)

> - KMD5 provided methods that took QString, but did not calculate the MD5
>   sum of the unicode values but converted it to latin1() first. thats
>   broken.

Ehhh.... this was deprecated and marked as such in the API! :)

> - it provided QByteArray and QCString variants. as QCString inherits
>   QByteArray, the QCString variants are useless.

Waldo already addressed this already.

> - I didn't quite like the FILE* variant and the way it reports errors in
>   a state variable. thats uncomfortable to use. I changed it in a QIODevice
>   (which QFile inherits) and a bool return value. more intuitive and
>   more consistent, and also allows you to pass it anything else that
>   inherits a QIODevice.

Great.  That is much better.  Another approach would have been to accept an
fd which would have solved all this to begin with.  But remember most of this
was just a port from a C implementation and was intended to keep as much
stuff compatiable.  In hind sight this was unnecessary, but then again in 
hindsight a lot of things that seem bad now looked good at the time :)

> - the error tracking of the class usage is awkward. I didn't find a single
>   usage of KMD5 that checks for these error states. IMHO if there are ways
>   to misuse the API, its a flaw in the API and not in the application.
>   Granted, the state nature of MD5 makes it difficult to design the API.

Indeed.  Even I, who ported/implemented this API, did not make use of 
this so it is really unnecessary and a flaw in the API.

>   I removed the finalize() method - its automatically finalized when
>   you request the digest() value. after that you cannot update the
>   digest anymore without calling reset() first.

Excellent.

>   This makes the class much easier to use in my opinion and is less
>   error-prone.

Agreed.

> - it forced the user to USE QUINT_8. This isn't widely used imho. I made
>   it accept char* and unsigned char*

Good.

> - it added two typedefs HASH and HEXHASH. These are not namespace clean
>   and do not belong in kde libs therefore. I removed HEXHASH completely
>   (its a QCString now for other reasons see below) and replaced HASH
>   with KMD5::Digest. This is more intuitive what it is and is namespace
>   clean.

Fine.  Note that this was simply another compatability layer for those that were using 
C-implementations and or the implementation provided by the spec... 

> - the documentation of the class was very incomplete, as some sentences
>   simply stopped in the middle without an ending or any contents. I
>   shortened the documentation and rewrote in large parts to match it the
>   actual behaviour and usage of the class.

I grant you the fact that the document was incomplete in some parts.  In fact
it did stop right in middle of sentences in a couple of places! And in few others
it did not make sense, but to call it very incomplete is a bit overboard.  It even 
showed examples of how to use the class and gave good descriptions int those
functions that are complete documented.

> - the documentation was wrong in many places. i.e. it said that
>   the user does not have to care about free'ing the rawDigest() return
>   value - which was not true and hence causes the leaks everywhere.

Hmm... now you're completely mixing things up!  It indeed leaked memory which is
very obvious since it does not free memory it created!  However, this function in 
no way shape or form claimed that you do not have to care about making a copy 
of rawDigest().  It is the other rawDigest function that stated that!  The problem
with that one is that the digest string would be corruppted once it left the scope
it was in IMHO!

> - to fix the leaks in an elegant way I changed the unprotected char*
>   return values of the digest() variants to QCString. IMHO we shouldn't
>   use unprotected char* returns in an API - let C coders do that, we
>   have better means in our language.

Good point.  Point taken...

> - the class has a private class pointer, but didn't have copy or assignment
>   operators. This wouldn't allow a BC extension. as I don't think its
>   useful to copy a KMD5 instance, I've disabled copying for now.
>   We could properly impelement copy and assignment operator though for
>   being able to keep binary compatibility.

This I did not know.  I had no idea that a copy ctor and/or an overload of the 
assignment operator was required to have a private pointer for keeping BC.  
Is this right ???

> - the method had a init() method that was _protected_. I fixed the API flaw
>   by making the method private.

I noticed that but I could not move it in the 2_2_BRANCH at the time...

> - the class leaked a file descriptor if there was an error reading the file.

Not if the user properly managed the the pointer he/she assigns, though this
indeed is a bad thing to provide as an API.  I have no I idea why I added that
to be honest with you...


> - probably a few other errors I forgot about already..

well...

> Unsolved problems:
>
> - The class is not completely endianess-safe it seems.

No it is not.  I have a couple of bug reports on it, but I did not have time
to look into it yet.  I will soon though...

> - the removal of the HASH and HEXHASH makes the patch source-incompatible.
>   however, I think its still worth the trouble as KMD5 isn't exactly
>   commonly used and it was a bad namespace breakage. We should fix it
>   before 3.0 (IMHO). these two typedefs didn't even match our
>   naming sheme. Opinions?

Yes, please remove it.

> - rawDigest() returns a const reference to its own state reference. its
>   not much of a problem as the digest won't ever change after rawDigest()
>   was called (the state is "finalized") then, however, when the class
>   is destructed the return value is dangling when you don't make a copy
>   of it. I'm not sure if that is a problem. it is not in the code I found,
>   they never store the digest() values over the lifetime of KMD5.

This will result in a corrupted text and will be very hard to catch for someone
at some point.  The documentation ought to warn the user! 

> Please have a look and comment if we can have a greatly fixed API in KDE 3
> or if the source incompatible changes are too much. Note I didn't even test
> if the class still works - however I didn't change anything in the "core
> calculation" routine - so it should still work fine. however I don't know a
> MD5 POP3 or webserver to test with :-/

I know HTTP MD5 test sites and I will test it with that once I finish compiling everything.

Thanks for fixing this up it now has a clean API.

Regards,
Dawit A.

[prev in list] [next in list] [prev in thread] [next in thread]