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

List:       kde-core-devel
Subject:    patch for KABC file plugin
From:       Carsten Pfeiffer <carpdjih () mailbox ! tu-berlin ! de>
Date:       2006-02-16 16:49:01
Message-ID: 200602161749.01872.carpdjih () mailbox ! tu-berlin ! de
[Download RAW message or body]

Hi,

while trying to find the cause for http://bugs.kde.org/show_bug.cgi?id=87163 
"kaddressbook empties resource on some conditions (data lost)", I found 
several problems in the KABC file plugin.

From looking at my log files, the problem in #87163 seems to be that KDirWatch 
notifies a change in the addressbook file (ResourceFile::fileChanged()), 
which causes the addresses to be cleared and asyncLoad() to be called. This 
loading apparently never succeeds during KDE shutdown, so the resource 
remains empty. A little later, the resource is saved again and the file is 
zeroed.

A fix for this is clear()ing the resource only right before loading the file, 
after all async IO has succeeded (clearAndLoad() in the patch). Additionally 
this prevents successive load() or loadAsync() calls from accumulating 
address entries (unless there are measures against this somewhere else). For 
extra safety, one could make a copy of mAddrMap before clear()ing and 
restoring it if format->load() doesn't succeed. Should we do this?

- "stale temp file detected"
After downloading with KIO, the KTempFile was not deleted, leading to the 
above debug message upon the next asyncLoad().

- always use KSaveFile for saving

- make sure to properly stop and start KDirWatch in all cases

- Synchronization issues:
Tickets are used to synchronize the saving of files. This should work with 
synchronous loading, but with async saving the ticket is released as soon as 
asyncSave returns. (See AddressBook::asyncSave()).
I have added a check to abort saving, if it's still in progress, but this 
obviously only helps this current ResourceFile instance, so a better solution 
is needed. Don't know who's affected by this.

Calling load() before loadAsync() has finished might also lead to troubles, so 
I added a guard for this.

Attached is a patch for the 3.5 branch. I'll also fix the net plugin later, 
which has similar issues.

Any comments?

Cheers
Carsten

["kabc.patch" (text/x-diff)]

Index: plugins/file/resourcefile.cpp
===================================================================
--- plugins/file/resourcefile.cpp	(Revision 510236)
+++ plugins/file/resourcefile.cpp	(Arbeitskopie)
@@ -207,6 +207,10 @@ bool ResourceFile::load()
 {
   kdDebug(5700) << "ResourceFile::load(): '" << mFileName << "'" << endl;
 
+  if ( d->mIsLoading ) {
+    abortAsyncLoading();
+  }
+  
   mAsynchronous = false;
 
   QFile file( mFileName );
@@ -215,11 +219,21 @@ bool ResourceFile::load()
     return false;
   }
 
-  return mFormat->loadAll( addressBook(), this, &file );
+  return clearAndLoad( &file );
+}
+
+bool ResourceFile::clearAndLoad( QFile *file )
+{
+  clear();
+  return mFormat->loadAll( addressBook(), this, file );
 }
 
 bool ResourceFile::asyncLoad()
 {
+  if ( d->mIsLoading ) {
+    abortAsyncLoading();
+  }
+    
   mAsynchronous = true;
 
   if ( mLocalTempFile ) {
@@ -244,26 +258,51 @@ bool ResourceFile::asyncLoad()
   return true;
 }
 
+void ResourceFile::abortAsyncLoading()
+{
+  kdDebug(5700) << "ResourceFile::abortAsyncLoading()" << endl;
+
+  if ( d->mLoadJob ) {
+    d->mLoadJob->kill(); // result not emitted
+    d->mLoadJob = 0;
+  }
+
+  delete mLocalTempFile;
+  mLocalTempFile = 0;
+  d->mIsLoading = false;
+}
+
+void ResourceFile::abortAsyncSaving()
+{
+  kdDebug(5700) << "ResourceFile::abortAsyncSaving()" << endl;
+    
+  if ( d->mSaveJob ) {
+    d->mSaveJob->kill(); // result not emitted
+    d->mSaveJob = 0;
+  }
+
+  d->mIsSaving = false;
+}
+
 bool ResourceFile::save( Ticket * )
 {
   kdDebug(5700) << "ResourceFile::save()" << endl;
 
+  if (d->mIsSaving) {
+    abortAsyncSaving();
+  }
+
   // create backup file
   QString extension = "_" + QString::number( QDate::currentDate().dayOfWeek() );
   (void) KSaveFile::backupFile( mFileName, QString::null /*directory*/,
                                 extension );
 
   mDirWatch.stopScan();
-  KSaveFile saveFile( mFileName );
-  bool ok = false;
-  if ( saveFile.status() == 0 && saveFile.file() )
-  {
-    mFormat->saveAll( addressBook(), this, saveFile.file() );
-    ok = saveFile.close();
-  }
 
+  bool ok = saveToFile( mFileName );
   if ( !ok )
     addressBook()->error( i18n( "Unable to save file '%1'." ).arg( mFileName ) );
+
   mDirWatch.startScan();
 
   return ok;
@@ -271,30 +310,44 @@ bool ResourceFile::save( Ticket * )
 
 bool ResourceFile::asyncSave( Ticket * )
 {
-  QFile file( mTempFile );
+  kdDebug(5700) << "ResourceFile::asyncSave()" << endl;
 
-  if ( !file.open( IO_WriteOnly ) ) {
-    emit savingError( this, i18n( "Unable to open file '%1'." ).arg( mTempFile ) );
-    return false;
+  if (d->mIsSaving) {
+    abortAsyncSaving();
   }
 
-  mDirWatch.stopScan();
-  mFormat->saveAll( addressBook(), this, &file );
-  file.close();
+  bool ok = saveToFile( mTempFile );
+  if ( !ok ) {
+    emit savingError( this, i18n( "Unable to save file '%1'." ).arg( mTempFile ) );
+    return false;
+  }
 
   KURL src, dest;
   src.setPath( mTempFile );
   dest.setPath( mFileName );
 
   KIO::Scheduler::checkSlaveOnHold( true );
-  d->mSaveJob = KIO::file_copy( src, dest, -1, true, false, false );
   d->mIsSaving = true;
+  mDirWatch.stopScan(); // restarted in uploadFinished()
+  d->mSaveJob = KIO::file_copy( src, dest, -1, true, false, false );
   connect( d->mSaveJob, SIGNAL( result( KIO::Job* ) ),
            this, SLOT( uploadFinished( KIO::Job* ) ) );
 
   return true;
 }
 
+bool ResourceFile::saveToFile( const QString& file )
+{
+  KSaveFile saveFile( file );
+  if ( saveFile.status() == 0 && saveFile.file() )
+  {
+    mFormat->saveAll( addressBook(), this, saveFile.file() );
+    return saveFile.close();
+  }
+
+  return false;
+}
+
 void ResourceFile::setFileName( const QString &fileName )
 {
   mDirWatch.stopScan();
@@ -328,10 +381,12 @@ QString ResourceFile::format() const
 
 void ResourceFile::fileChanged()
 {
+    kdDebug(5700) << "ResourceFile::fileChanged(): " << mFileName << endl;
+
   if ( !addressBook() )
     return;
 
-  clear();
+//  clear(); // moved to clearAndLoad()
   if ( mAsynchronous )
     asyncLoad();
   else {
@@ -354,23 +409,32 @@ void ResourceFile::downloadFinished( KIO
 {
   d->mIsLoading = false;
 
-  if ( !mLocalTempFile )
+  if ( !mLocalTempFile ) {
     emit loadingError( this, i18n( "Download failed in some way!" ) );
+    return;
+  }
 
   QFile file( mTempFile );
   if ( !file.open( IO_ReadOnly ) ) {
     emit loadingError( this, i18n( "Unable to open file '%1'." ).arg( mTempFile ) );
+    delete mLocalTempFile;
+    mLocalTempFile = 0;
     return;
   }
 
-  if ( !mFormat->loadAll( addressBook(), this, &file ) )
+  if ( !clearAndLoad( &file ) )
     emit loadingError( this, i18n( "Problems during parsing file '%1'." ).arg( mTempFile ) );
   else
     emit loadingFinished( this );
+
+  delete mLocalTempFile;
+  mLocalTempFile = 0;
 }
 
 void ResourceFile::uploadFinished( KIO::Job *job )
 {
+  kdDebug(5700) << "ResourceFile::uploadFinished()" << endl;
+
   d->mIsSaving = false;
 
   if ( job->error() )
Index: plugins/file/resourcefile.h
===================================================================
--- plugins/file/resourcefile.h	(Revision 510236)
+++ plugins/file/resourcefile.h	(Arbeitskopie)
@@ -28,6 +28,7 @@
 
 #include <kabc/resource.h>
 
+class QFile;
 class QTimer;
 
 class KTempFile;
@@ -148,6 +149,11 @@ class KABC_EXPORT ResourceFile : public 
     void unlock( const QString &fileName );
 
   private:
+    bool saveToFile( const QString& file );
+    void abortAsyncLoading();
+    void abortAsyncSaving();
+    bool clearAndLoad( QFile *file );
+
     QString mFileName;
     QString mFormatName;
 


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