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

List:       kde-commits
Subject:    Re: kdesupport/mimelib
From:       Don Sanders <don () sanders ! org>
Date:       2000-04-18 2:49:45
[Download RAW message or body]

On Tue, 18 Apr 2000, Waldo Bastian wrote:
> On Mon, 17 Apr 2000, Don Sanders wrote:
> > On Mon, 17 Apr 2000, CVS by waba wrote:
> > > kdesupport/mimelib datetime.cpp,1.6,1.7
> > > Author: waba
> > > CVSROOT: /home/kde
> > > Mon Apr 17 08:47:51 MET DST 2000
> > > Update of /home/kde/kdesupport/mimelib
> > > In directory zeus:/tmp/cvs-serv13521
> > > 
> > > Modified Files:
> > > 	datetime.cpp
> > > Log Message:
> > > WABA: Fixed 64-bit bug with time_t.
> > 
> > Waldo are you sure about this fix?
> 
> Ehm.. well, I was :-)

To cut a medium length story short I looked at the code more carefully and I
guess I don't have any objections.

Full length story follows:

> > For a long time KMail wasn't working on the Alpha architecture, it was
> > crashing when a composer window was created. This was due to a KMail bug
> > rather than a mimelib bug, I fixed this in the HEAD branch and later in
> > KMail 1.0.29.1.
> > 
> > A week or so after I fixed it in the HEAD branch someone else independently
> > fiixed it, but they fixed it in a different way. I have a vague
> > recollection of this other fix being same as the one you have committed.
> > 
> > This fix may work but I don't believe it is necessary (the real bug was in
> > KMail) and it doesn't look right to me.
> 
> I dunno if this fixes a crash or not, but passing this DwUint32 to functions 
> which expect a time_t is IMO asking for trouble. When/if time_t happens to be 
> 64-bit this function might write 64 bit into a 32 bit space and I have no 
> idea where the other 32 bits go.

My thinking too. Looking at the affected code

<quote>
 void DwDateTime::_FromUnixTime(DwUint32 aTime) {
-    DwUint32 t = aTime + mZone*60;
+    time_t t = aTime + mZone*60;
 #if defined(USE_PORTABLE_GMTIME)
     struct tm tt
     my_gmtime_r(&t, &tt);
#elif defined(HAVE_GMTIME_R)							
    struct tm tt								    
    gmtime_r(&t, &tt);								    
#else										
    struct tm tt = *gmtime((time_t*)&t);					    
#endif										
</quote>

If USE_PORTABLE_GMTIME is defined then after your changes a *time_t is being
passed into a function 
  static int my_gmtime_r(DwUint32 *pt, struct tm *ptms);
that expects a *DwUint32 (which is not so good).

But now I notice that in the case that USE_PORTABLE_GMTIME isn't defined then
your change is indeed an improvement (as both gmtime_r and gmtime expect a
*time_t at least according to my man pages for the Solaris system I'm using).

Unfortunately I don't know under what conditions USE_PORTABLE_GMTIME time is
defined. 

Maybe 

 void DwDateTime::_FromUnixTime(DwUint32 aTime) {
-    DwUint32 t = aTime + mZone*60;
 #if defined(USE_PORTABLE_GMTIME)
+    DwUint32 t = aTime + mZone*60;
     struct tm tt
     my_gmtime_r(&t, &tt);
#elif defined(HAVE_GMTIME_R)							
+    time_t t = aTime + mZone*60;
    struct tm tt								    
    gmtime_r(&t, &tt);								    
#else										
+    time_t t = aTime + mZone*60;
    struct tm tt = *gmtime((time_t*)&t);					    
#endif										

might be better (though I haven't tested this and could be wrong).


> Looking better at the code, it is interesting to see that it has all these 
> #if define(HAVE_GMTIME_R) and #if defined(USE_PORTABLE_GMTIME)  checks but 
> that nobody ever defines these things. (At least not in the HEAD branch)

Ok.

> 
> Cheers,
> Waldo

Here is the change I made to KMail a few months ago to fix the Alpha bug

http://www.nebsllc.com/cgi-bin/cvsweb.cgi/kdenetwork/kmail/kmmessage.cpp.diff?r1=1.68.2.9&r2=1.68.2.10&sortby=date&only_with_tag=KDE_1_1_BRANCH


<quote>
RCS file: /kdecvs/mirror/kdenetwork/kmail/kmmessage.cpp,v
retrieving revision 1.68.2.9retrieving revision 1.68.2.10
diff -u -r1.68.2.9 -r1.68.2.10
--- kdenetwork/kmail/kmmessage.cpp	1999/08/11 07:58:12	1.68.2.9
+++ kdenetwork/kmail/kmmessage.cpp	2000/03/17 07:35:39	1.68.2.10
@@ -720,7 +720,7 @@ void KMMessage::setDate(time_t aDate) {
   KMMessageInherited::setDate(aDate);
-  mMsg->Headers().Date().FromUnixTime(aDate);
+  mMsg->Headers().Date().FromCalendarTime(aDate);
   mMsg->Headers().Date().Assemble();   mNeedsAssembly = TRUE;   mDirty = TRUE;
</quote>

Where in mimelib/datetime.cpp
void DwDateTime::FromUnixTime(DwUint32 aTime)
void DwDateTime::FromCalendarTime(time_t aTime)

So before my change KMail was passing a time_t into a function that expected a
DwUint32. (And this caused KMail to crash on Alpha).

KMail no longer uses the FromUnixTime function so I guess it won't be
affected by your change. So this discussion isn't of critical
importance. But I felt like looking into a bit more anyway.

BFN,
Don.


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