[Bug-ddrescue] 2 Suggestions (bi-directional retry and user interruption exit code)

[Florian Sedivy]

First of all I want to express my deepest appreciation for ddrescue. Although 
it follows a very simple concept (make a 1-to-1 copy), it is nevertheless 
continually refined year after year. Thank you very much!

I have two suggestions. First is a very simple change of the exit status after 
user interruption. Currently this is 0, which makes it indistinguishable from a 
finished run. In the context of a script and/ or loop it is very useful to 
catch a user interruption in order to exit the loop or script. 
The fix is easy: delete line 739 (20 from end) of rescuebook.cc and exit status 
will be −1.
738          if( retval == 0 ) current_status( finished );
739     -    else if( retval < 0 ) retval = 0;          // interrupted by user
Unless of course there is a special reason why 0 has to be the return value. I 
used this patch myself and it helped a lot when I was emulating the 
functionality of my second proposition with a simple shell one-liner. 

The other suggestion is about retry mode. I wish ddrescue could automatically 
alternate the reading direction for each pass when retrying 2 or more times. My 
proposal is to trigger this behavior by specifying negative numbers with the -r 
--retry-passes option. For compatibility −1 could still mean infinity (as there 
is no way alternating a single pass). With numbers −2 or smaller ddrescue would 
toggle the direction on every even pass to the opposite of what had been 
specified on the command line. (There would be no way to specify infinite 
alternation, but that is acceptable I guess.) 

When trying to implement that solution I changed main.cc:
588     –      case 'r': rb_opts.max_retries = getnum( arg, 0, -1, INT_MAX ); 
break;
588     +      case 'r': rb_opts.max_retries = getnum( arg, 0, INT_MIN, INT_MAX 
); break;

Rescuebook::do_rescue
717     – retval = reverse ? rcopy_errors() : copy_errors(); 
717     + retval = copy_errors(); 

and mainly Rescuebook::copy_errors, eliminating the need for 
Rescuebook::rcopy_errors
449ff
// Return values: 1 I/O error, 0 OK, -1 interrupted, -2 logfile error.
// Try to read the damaged areas, one sector at a time.
//
int Rescuebook::copy_errors()
  {
   char msgbuf[80] = "Retrying bad sectors... Retry ";
   const int msglen = std::strlen( msgbuf );
 
-  for( int retry = 1; max_retries < 0 || retry <= max_retries; ++retry )
+  for( int retry = 1; max_retries == -1 || retry <= abs (max_retries); ++retry 
)
     {
     long long pos = 0;
+    long long end = LLONG_MAX;
     bool first_post = true, block_found = false;
     snprintf( msgbuf + msglen, ( sizeof msgbuf ) - msglen, "%d", retry );
     read_logger.print_msg( t1 - t0, msgbuf );
 
     if( retry == 1 && current_status() == retrying &&
-        domain().includes( current_pos() ) )
+        domain().includes( current_pos() - reverse) )
       {
-      Block b( current_pos(), 1 );
+      Block b( current_pos() - reverse, 1 );
       find_chunk( b, Sblock::bad_sector, domain(), hardbs() );
-      if( b.size() > 0 ) pos = b.pos();                // resume
+      if( b.size() > 0 ) { pos = b.pos(); end = b.end(); }     // resume
       }
+    if( reverse ) 
+      {
+      while( end > 0 )
+        {
+        long long pos = end - hardbs();
+        if( pos < 0 ) pos = 0;
+        Block b( pos, end - pos );
+        rfind_chunk( b, Sblock::bad_sector, domain(), hardbs() );
+        if( b.size() <= 0 ) break;
+        end = b.pos();
+        current_status( retrying );
+        block_found = true;
+        int copied_size = 0, error_size = 0;
+        const int retval = copy_and_update( b, Sblock::bad_sector, copied_size,
+                                          error_size, msgbuf, first_post, 
false );
+        if( copied_size > 0 ) errsize -= copied_size;
+        if( retval ) return retval;
+        if( error_size > 0 ) error_rate += error_size;
+        update_rates();
+        if( !update_logfile( odes_ ) ) return -2;
+        }
+      }
+    else
+      {
       while( pos >= 0 )
         {
         Block b( pos, hardbs() );
         find_chunk( b, Sblock::bad_sector, domain(), hardbs() );
         if( b.size() <= 0 ) break;
         pos = b.end();
         current_status( retrying );
         block_found = true;
         int copied_size = 0, error_size = 0;
         const int retval = copy_and_update( b, Sblock::bad_sector, copied_size,
                                           error_size, msgbuf, first_post, true 
);
         if( copied_size > 0 ) errsize -= copied_size;
         if( retval ) return retval;
         if( error_size > 0 ) error_rate += error_size;
         update_rates();
         if( !update_logfile( odes_ ) ) return -2;
         }
+      }
     if( !block_found || retry >= INT_MAX ) break;
+    if( max_retries < -1 ) reverse = !reverse;
     }
   return 0;
   }

I could not really test that code. Also I would have liked to insert a 5 second 
pause between passes, but don't know how to accomplish that in a platform 
independent way and without messing up the read statistics. 

Greetings, 
Florian