Bug 82843 - MD5sum calculated twice when burning image
Summary: MD5sum calculated twice when burning image
Status: RESOLVED FIXED
Alias: None
Product: k3b
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Debian testing Linux
: NOR normal with 31 votes (vote)
Target Milestone: ---
Assignee: Sebastian Trueg
URL:
Keywords:
: 88841 125544 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-06-04 18:41 UTC by Maik Zumstrull
Modified: 2006-10-21 12:55 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to fix bug 82843 (4.98 KB, patch)
2005-10-08 22:25 UTC, Shane Frasier
Details
Patch to fix bug 82843 (5.99 KB, patch)
2005-10-08 23:00 UTC, Shane Frasier
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Maik Zumstrull 2004-06-04 18:41:24 UTC
Version:           0.11.9 (using KDE KDE 3.2.2)
Installed from:    Debian testing/unstable Packages
OS:                Linux

When using the "Burn CD image" function of k3b to burn a .iso File, and a .iso.md5 file exists, k3b verifies the images MD5sum. When burning with "verify written data" afterwards, k3b calculates the MD5sum of the image again. I think that's a waste of time, if the MD5sum is a match in the first test, it should be used when verifying the written disk.
Comment 1 jos poortvliet 2004-06-15 11:45:46 UTC
I agree. this is a waste of time.
Comment 2 Adrien Cordonnier 2005-04-25 00:41:55 UTC
Several wishes are related to this bug:
- Bug 59602, multiple copies of a cd
- Bug 77312, function to compare (verify) project with CD/DVD content
- Bug 77432, option to save a .md5sum file on every DataCD/DVD

Currently, the k3b behavior is to compute the image md5sum when loading the image and again each time the md5sum is needed. This is quite annoying -- with the fact the tray is opened between the burning and the check -- especially for multiple copies.

I think the behavior should be to compute the md5sum only once when the image is loaded or created. If a .iso.md5sum file with the same name exists, it should be used to verify the download. The md5sum should then be stored with the possibility to save it for later verifications. If a verification as been requested for after the burning, the stored md5sum should be used.
Comment 3 Adrien Cordonnier 2005-04-25 00:45:29 UTC
*** This bug has been confirmed by popular vote. ***
Comment 4 Shane Frasier 2005-10-08 22:25:24 UTC
Created attachment 12917 [details]
Patch to fix bug 82843

I created this patch that appears to fix this bug.  Please let me know the
results if you test it.
Comment 5 Shane Frasier 2005-10-08 22:58:54 UTC
Comment on attachment 12917 [details]
Patch to fix bug 82843

Better patch submitted
Comment 6 Shane Frasier 2005-10-08 23:00:42 UTC
Created attachment 12918 [details]
Patch to fix bug 82843

Thsi is a better patch that the one I submitted just before.  I made the
corresponding changes for burning DVDs as well.
Comment 7 Sebastian Trueg 2005-10-27 22:22:47 UTC
*** Bug 88841 has been marked as a duplicate of this bug. ***
Comment 8 kavol 2006-02-25 09:20:09 UTC
Just to add to comment 2 ... you talk about ".iso.md5sum" - I think that ".md5" (with and without ".iso" part) is much more common than ".md5sum" ... but the program should support all well-known naming schemes and, if possible, other checksuming formats as well (see bug 112270).
Comment 9 Jouni Ryhänen 2006-06-04 13:02:03 UTC
I think checking md5sums should be optional. I never need to check the sums and that's why I can't use K3B to burn images. I just takes too much time and stresses my hard drive.
Comment 10 Sebastian Trueg 2006-06-04 13:40:06 UTC
nonesense. Just click the start button and k3b will cancel the md5sum 
calculation.
Comment 11 Christoph Burger-Scheidlin 2006-09-22 19:15:08 UTC
*** Bug 125544 has been marked as a duplicate of this bug. ***
Comment 12 Peter Missel 2006-10-17 19:22:31 UTC
Sebastian, sure, the /initial/ md5 calculation can be cancelled. But when enabling "Verify", the md5sum is /again/ calculated for /every/ copy of the disk.
So, for making five copies of an .iso image with verify, the .iso file is being read ten times - five times for the burn, and another five times for calculating the md5sum.

k3b should /remember/ the md5sum of the image it is currently using, that is all.
Comment 13 Sebastian Trueg 2006-10-21 10:04:31 UTC
SVN commit 597651 by trueg:

Use the ChecksumPipe to calculate the checksum of an iso image written to CD/DVD instead
of calculating it afterwards.

BUG: 82843
BUG: 134021


 M  +1 -0      jobs/k3bclonetocreader.cpp  
 M  +27 -4     jobs/k3biso9660imagewritingjob.cpp  
 M  +3 -0      jobs/k3biso9660imagewritingjob.h  
 M  +39 -39    jobs/k3bisoimageverificationjob.cpp  
 M  +2 -0      jobs/k3bisoimageverificationjob.h  
 M  +59 -12    tools/k3bchecksumpipe.cpp  
 M  +22 -3     tools/k3bchecksumpipe.h  
 M  +8 -2      tools/k3bmd5job.cpp  


--- trunk/extragear/multimedia/k3b/libk3b/jobs/k3bclonetocreader.cpp #597650:597651
@@ -78,6 +78,7 @@
 
     struct tocheader* th = (struct tocheader*)buffer;
     int dataLen = K3bDevice::from2Byte( th->len ) + 2;  // the len field does not include it's own length
+    kdDebug() << "(K3bCloneTocReader) dataLen+2 = " << dataLen << endl;
 
     if( th->first != 1 ) {
       kdDebug() << "(K3bCloneTocReader) first session != 1" << endl;
--- trunk/extragear/multimedia/k3b/libk3b/jobs/k3biso9660imagewritingjob.cpp #597650:597651
@@ -27,6 +27,7 @@
 #include <k3bcore.h>
 #include <k3bversion.h>
 #include <k3bexternalbinmanager.h>
+#include <k3bchecksumpipe.h>
 
 #include <kdebug.h>
 #include <kconfig.h>
@@ -40,6 +41,14 @@
 #include <qapplication.h>
 
 
+class K3bIso9660ImageWritingJob::Private
+{
+public:
+  K3bChecksumPipe checksumPipe;
+  QFile imageFile;
+};
+
+
 K3bIso9660ImageWritingJob::K3bIso9660ImageWritingJob( K3bJobHandler* hdl )
   : K3bBurnJob( hdl ),
     m_writingMode(K3b::WRITING_MODE_AUTO),
@@ -53,11 +62,13 @@
     m_copies(1),
     m_verifyJob(0)
 {
+  d = new Private;
 }
 
 K3bIso9660ImageWritingJob::~K3bIso9660ImageWritingJob()
 {
   delete m_tocFile;
+  delete d;
 }
 
 
@@ -95,6 +106,8 @@
     return;
   }
 
+  d->checksumPipe.close();
+
   if( success ) {
     if( !m_simulate && m_verifyData ) {
       emit burning(false);
@@ -112,6 +125,7 @@
       }
       m_verifyJob->setDevice( m_device );
       m_verifyJob->setImageFileName( m_imagePath );
+      m_verifyJob->setImageMD5Sum( d->checksumPipe.checksum() );
       if( m_copies == 1 )
 	emit newTask( i18n("Verifying written data") );
       else
@@ -253,9 +267,18 @@
     return;
   }
 
+  // we simply always calculate the checksum, thus making the code simpler
+  d->imageFile.close();
+  d->imageFile.setName( m_imagePath );
+  d->imageFile.open( IO_ReadOnly );
+  d->checksumPipe.close();
+  d->checksumPipe.readFromFd( d->imageFile.handle() );
+
   if( prepareWriter( media ) ) {
     emit burning(true);
     m_writer->start();
+    d->checksumPipe.writeToFd( m_writer->fd(), true );
+    d->checksumPipe.open( K3bChecksumPipe::MD5, true );
   }
   else {
     m_finished = true;
@@ -319,7 +342,8 @@
       else
 	writer->addArgument("-data");
 
-      writer->addArgument( m_imagePath );
+      // read from stdin
+      writer->addArgument( "-" );
 
       m_writer = writer;
     }
@@ -349,7 +373,7 @@
 	  *s << "\n";
 	  *s << "TRACK MODE1" << "\n";
 	}
-	*s << "DATAFILE \"" << m_imagePath << "\" 0 \n";
+	*s << "DATAFILE \"-\" 0 \n";
 
 	m_tocFile->close();
       }
@@ -382,13 +406,12 @@
     writer->setSimulate( m_simulate );
     writer->setBurnSpeed( m_speed );
     writer->setWritingMode( m_writingMode == K3b::DAO ? K3b::DAO : 0 );
-    writer->setImageToWrite( m_imagePath );
+    writer->setImageToWrite( QString::null ); // read from stdin
     writer->setCloseDvd( !m_noFix );
 
     m_writer = writer;
   }
 
-
   connect( m_writer, SIGNAL(infoMessage(const QString&, int)), this, SIGNAL(infoMessage(const QString&, int)) );
   connect( m_writer, SIGNAL(nextTrack(int, int)), this, SLOT(slotNextTrack(int, int)) );
   connect( m_writer, SIGNAL(percent(int)), this, SLOT(slotWriterPercent(int)) );
--- trunk/extragear/multimedia/k3b/libk3b/jobs/k3biso9660imagewritingjob.h #597650:597651
@@ -90,6 +90,9 @@
   int m_currentCopy;
 
   K3bIsoImageVerificationJob* m_verifyJob;
+
+  class Private;
+  Private* d;
 };
 
 #endif
--- trunk/extragear/multimedia/k3b/libk3b/jobs/k3bisoimageverificationjob.cpp #597650:597651
@@ -1,10 +1,10 @@
 /* 
  *
  * $Id$
- * Copyright (C) 2003 Sebastian Trueg <trueg@k3b.org>
+ * Copyright (C) 2003-2006 Sebastian Trueg <trueg@k3b.org>
  *
  * This file is part of the K3b project.
- * Copyright (C) 1998-2004 Sebastian Trueg <trueg@k3b.org>
+ * Copyright (C) 1998-2006 Sebastian Trueg <trueg@k3b.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -40,6 +40,7 @@
 
   bool canceled;
   bool needToCalcMd5;
+  bool imageMd5SumDone;
   K3bMd5Job* md5Job;
   K3bDevice::Device* device;
   QString imageFileName;
@@ -101,12 +102,19 @@
 
   d->canceled = false;
   d->needToCalcMd5 = d->imageMd5Sum.isEmpty();
+  d->imageMd5SumDone = false;
 
+  d->imageSize = K3b::filesize( d->imageFileName );
+  if( d->imageSize == (KIO::filesize_t)0 ) {
+    emit infoMessage( i18n("Unable to determine size of file %1.").arg(d->imageFileName), ERROR );
+    finishVerification(false);
+  }
+
   // first we need to reload and mount the device
   emit newTask( i18n("Reloading the medium") );
 
   connect( K3bDevice::reload( d->device ), SIGNAL(finished(bool)),
-	     this, SLOT(slotMediaReloaded(bool)) );
+	   this, SLOT(slotMediaReloaded(bool)) );
 }
 
 
@@ -117,27 +125,42 @@
 			 i18n("Unable to Close the Tray") );
 
   if( d->needToCalcMd5 ) {
-    emit newTask( i18n("Reading original data") );
-    
-    // start it
-    d->md5Job->setFile( d->imageFileName );
-    d->md5Job->start();
+    calculateImageChecksum();
   }
   else {
-    slotMd5JobFinished( true );
+    d->imageMd5SumDone = true;
+    calculateWrittenDataChecksum();
   }
 }
 
 
-void K3bIsoImageVerificationJob::slotMd5JobFinished( bool success )
+void K3bIsoImageVerificationJob::calculateImageChecksum()
 {
-  if( d->canceled ) {
+  emit newTask( i18n("Reading original data") );
+  d->md5Job->setFile( d->imageFileName );
+  d->md5Job->start();
+}
+
+
+void K3bIsoImageVerificationJob::calculateWrittenDataChecksum()
+{
+  emit newTask( i18n("Reading written data") );
+  if( !d->device->open() ) {
+    emit infoMessage( i18n("Unable to open device %1.").arg(d->device->blockDeviceName()), ERROR );
     finishVerification(false);
   }
+  else {
+    d->md5Job->setDevice( d->device );
+    d->md5Job->setMaxReadSize( d->imageSize );
+    d->md5Job->start();
+  }
+}
 
-  if( success ) {
 
-    if( !d->imageMd5Sum.isEmpty() ) {
+void K3bIsoImageVerificationJob::slotMd5JobFinished( bool success )
+{
+  if( success && !d->canceled ) {
+    if( d->imageMd5SumDone ) {
       // compare the two sums
       if( d->imageMd5Sum != d->md5Job->hexDigest() ) {
 	emit infoMessage( i18n("Written data differs from original."), ERROR );
@@ -149,32 +172,9 @@
       }
     }
     else {
-      
       d->imageMd5Sum = d->md5Job->hexDigest();
-
-      //
-      // now we need to calculate the md5sum of the written image
-      // since it is possible that the image has been padded while writing we need
-      // the image filesize to make sure we do not compare too much data and thus get
-      // a wrong result
-      //
-
-      d->imageSize = K3b::filesize( d->imageFileName );
-      if( d->imageSize == (KIO::filesize_t)0 ) {
-	emit infoMessage( i18n("Unable to determine size of file %1.").arg(d->imageFileName), ERROR );
-	finishVerification(false);
-      }
-      else if( !d->device->open() ) {
-	emit infoMessage( i18n("Unable to open device %1.").arg(d->device->blockDeviceName()), ERROR );
-	finishVerification(false);
-      }
-      else {
-	// start the written data check
-	emit newTask( i18n("Reading written data") );
-	d->md5Job->setDevice( d->device );
-	d->md5Job->setMaxReadSize( d->imageSize );
-	d->md5Job->start();
-      }
+      d->imageMd5SumDone = true;
+      calculateWrittenDataChecksum();
     }
   }
   else {
@@ -186,7 +186,7 @@
 
 void K3bIsoImageVerificationJob::slotMd5JobProgress( int p )
 {
-  if( d->needToCalcMd5 && !d->imageMd5Sum.isEmpty() )
+  if( d->needToCalcMd5 && d->imageMd5SumDone )
     emit percent( 50 + p/2 );
   else if( d->needToCalcMd5 )
     emit percent( p/2 );
--- trunk/extragear/multimedia/k3b/libk3b/jobs/k3bisoimageverificationjob.h #597650:597651
@@ -39,6 +39,8 @@
   void setImageMD5Sum( const QCString& );
 
  private slots:
+  void calculateImageChecksum();
+  void calculateWrittenDataChecksum();
   void slotMediaReloaded( bool success );
   void slotMd5JobFinished( bool success );
   void slotMd5JobProgress( int p );
--- trunk/extragear/multimedia/k3b/libk3b/tools/k3bchecksumpipe.cpp #597650:597651
@@ -29,7 +29,10 @@
 {
 public:
   Private() :
-    fdToWriteTo(-1) {
+    fdToReadFrom(-1),
+    fdToWriteTo(-1),
+    closeFdToReadFrom(false),
+    closeFdToWriteTo(false) {
   }
 
   void run() {
@@ -37,19 +40,20 @@
     buffer.resize( 10*2048 );
     ssize_t r = 0;
     ssize_t total = 0;
-    while( ( r = ::read( pipeIn.out(), buffer.data(), buffer.size() ) ) > 0 ) {
+    while( ( r = ::read( readFd(), buffer.data(), buffer.size() ) ) > 0 ) {
 
       // write it out
       ssize_t w = 0;
       ssize_t ww = 0;
       while( w < r ) {
-	if( ( ww = ::write( fdToWriteTo == -1 ? pipeOut.in() : fdToWriteTo,
-			    buffer.data()+w, r-w ) ) > 0 ) {
+	if( ( ww = ::write( writeFd(), buffer.data()+w, r-w ) ) > 0 ) {
 	  w += ww;
 	}
 	else {
 	  kdDebug() << "(K3bChecksumPipe) write failed." << endl;
-	  break;
+	  if( closeWhenDone )
+	    close();
+	  return;
 	}
       }
 
@@ -59,6 +63,9 @@
       total += r;
     }
     kdDebug() << "(K3bChecksumPipe) thread done: " << r << " (total bytes: " << total << ")" << endl;
+
+    if( closeWhenDone )
+      close();
   }
 
   void update( const char* in, int len ) {
@@ -77,12 +84,44 @@
     }
   }
 
+  int readFd() const {
+    if( fdToReadFrom == -1 )
+      return pipeIn.out();
+    else
+      return fdToReadFrom;
+  }
+
+  int writeFd() const {
+    if( fdToWriteTo == -1 )
+      return pipeOut.in();
+    else
+      return fdToWriteTo;
+  }
+
+  void close() {
+    pipeIn.close();
+    pipeOut.close();
+
+    if( fdToWriteTo != -1 &&
+	closeFdToWriteTo )
+      ::close( fdToWriteTo );
+
+    if( fdToReadFrom != -1 &&
+	closeFdToReadFrom )
+      ::close( fdToReadFrom );
+  }
+
   int checksumType;
 
+  int fdToReadFrom;
   int fdToWriteTo;
   K3bPipe pipeIn;
   K3bPipe pipeOut;
 
+  bool closeWhenDone;
+  bool closeFdToReadFrom;
+  bool closeFdToWriteTo;
+
   KMD5 md5;
 
   QByteArray buffer;
@@ -101,13 +140,15 @@
 }
 
 
-bool K3bChecksumPipe::open( int type )
+bool K3bChecksumPipe::open( int type, bool closeWhenDone )
 {
-  close();
+  if( d->running() )
+    return false;
 
   d->checksumType = type;
+  d->closeWhenDone = closeWhenDone;
 
-  if( !d->pipeIn.open() ) {
+  if( d->fdToReadFrom == -1 && !d->pipeIn.open() ) {
     return false;
   }
 
@@ -126,17 +167,22 @@
 void K3bChecksumPipe::close()
 {
   d->pipeIn.closeIn();
-
   d->wait();
+  d->close();
+}
 
-  d->pipeIn.close();
-  d->pipeOut.close();
+
+void K3bChecksumPipe::readFromFd( int fd, bool close )
+{
+  d->fdToReadFrom = fd;
+  d->closeFdToReadFrom = close;
 }
 
 
-void K3bChecksumPipe::writeToFd( int fd )
+void K3bChecksumPipe::writeToFd( int fd, bool close )
 {
   d->fdToWriteTo = fd;
+  d->closeFdToWriteTo = close;
 }
 
 
@@ -161,3 +207,4 @@
 
   return QCString();
 }
+
--- trunk/extragear/multimedia/k3b/libk3b/tools/k3bchecksumpipe.h #597650:597651
@@ -16,7 +16,6 @@
 #ifndef _K3B_CHECKSUM_PIPE_H_
 #define _K3B_CHECKSUM_PIPE_H_
 
-
 #include <qcstring.h>
 
 /**
@@ -36,18 +35,38 @@
   /**
    * Opens the pipe and thus starts the 
    * checksum calculation
+   *
+   * \param closeWhenDone If true the pipes will be closed
+   *        once all data has been read.
    */
-  bool open( int type = MD5 );
+  bool open( int type = MD5, bool closeWhenDone = false );
 
   /**
    * Close the pipe and finish the checksum.
    */
   void close();
 
-  void writeToFd( int fd );
+  /**
+   * Set the file descriptor to read from. If this is -1 (the default) then
+   * data has to be piped into the in() file descriptor.
+   *
+   * \param fd The file descriptor to read from.
+   * \param close If true the reading file descriptor will be closed on a call to close()
+   */
+  void readFromFd( int fd, bool close = false );
 
   /**
+   * Set the file descriptor to write to. If this is -1 (the default) then
+   * data has to read from the out() file descriptor.
+   *
+   * \param fd The file descriptor to write to.
+   * \param close If true the reading file descriptor will be closed on a call to close()
+   */
+  void writeToFd( int fd, bool close = false );
+
+  /**
    * The file descriptor to write into
+   * Only valid if no fd to read from has been set
    */
   int in() const;
 
--- trunk/extragear/multimedia/k3b/libk3b/tools/k3bmd5job.cpp #597650:597651
@@ -1,10 +1,10 @@
 /* 
  *
  * $Id$
- * Copyright (C) 2003 Sebastian Trueg <trueg@k3b.org>
+ * Copyright (C) 2003-2006 Sebastian Trueg <trueg@k3b.org>
  *
  * This file is part of the K3b project.
- * Copyright (C) 1998-2004 Sebastian Trueg <trueg@k3b.org>
+ * Copyright (C) 1998-2006 Sebastian Trueg <trueg@k3b.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -114,6 +114,12 @@
   else
     d->imageSize = 0;
 
+  if( d->device ) {
+    //
+    // Let the drive determine the optimal reading speed
+    //
+    d->device->setSpeed( 0xffff, 0xffff );
+  }
     
   d->md5.reset();
   d->finished = false;
Comment 14 jos poortvliet 2006-10-21 12:55:19 UTC
nice work sebas. thanx.