[Bug-tar] Wrong "size left" field value in chunk header in multi-volume

[Marek Kielar]

1. I encountered a bug while making my own test using multi-volume - I provide 
a test case and I propose a solution at least for this case.
2. While passing through the code (which, I must say, executes in a quite 
wicked way for multi-volume) I also questioned some other fragments.
3. I also have a few questions.
4. buffer.c diff output.

Source file altered: buffer.c
Functions altered: _gnu_flush_write, simple_flush_write.
All proposed solutions take into consideration only the bug-oriented point of 
view - even though they are quite certainly correct in this case, they may not 
be relevant or appropriate in other cases.

1. The bug

1.1. The test case
Tested in 1.15, 1.19 and 1.20 (current).
The test case consists of one file of length of 20139 - tested with all-spaces.
tar executed with "-cf archive -ML 1 file".
Creates three chunks - two first are correct, the third has a corrupted header 
- wrong value of "size left" written.
This leads tar executed with "-xf archive -ML 1" or "-tf archive -ML 1" to 
failure with "This volume is out of sequence" message.

1.2. The cause
The problem is caused by wrong value in real_s_sizeleft while writing the chunk 
header. For unknown reason to me this value is sometimes correct.

1.3. Proposed solution
Assigning real_s_sizeleft proper value in _gnu_flush_write, using size_left, 
and later (for the sake of safety) restoring size_left original value.


2. Other code

2.1. Records_written are counted wrong (due to "wicked-way execution" - twice 
too often) in _gnu_flush_write and simple_flush_write. Proposed change.

2.2. Copy_ptr and copy_size are calculated (logically) wrong - though it 
(un)fortunately does not make a change in execution. Proposed change.


3. Questions
I supply source location for each question.

3.1.
tar.c:
Why DEFAULT_BLOCKING is made equal to 20? Should not it be 2, taking into 
consideration that "tape-length-size step" is 2*BLOCKSIZE?

3.2.
buffer.c: gnu_add_multi_volume_header: 1392
finish_header() in 1390 already does that

3.3.
buffer.c: flush_archive() . record_end variable
What is the purpose of reassigning the same value to record_end, if it never 
changes without record_start, and here it is drawed from record_start.
If this is a development thing - in case of record_end pointer increase, the 
record_buffer is not altered.

3.4.
buffer.c: find_next_block() . hit_eof variable
How does EOF-trapping work for (current_block == record_end), if in buffer.c: 
flush_archive() we have an assignment of "current_block = record_start" and 
current_block is never changed?

3.5.
buffer.c: _gnu_flush_write() vs simple_flush_write()
Why not incorporate a simple_flush_write() call into _gnu_flush_write()?

3.6.
I would say that there is an inconsequence in that space-indents and 
tab-indents are use simultaneously - tabs do not necessarily equal to 8 spaces.


4. buffer.c diff output

--- buffer_old.c        2008-02-04 11:36:52.000000000 +0100
+++ buffer.c    2008-07-24 11:50:10.000000000 +0200
@@ -1485,7 +1485,8 @@
     archive_write_error (status);
   else
     {
-      records_written++;
+      if ( status > 0 )
+       records_written++;
       bytes_written += status;
     }
 }
@@ -1565,13 +1566,15 @@
   char *copy_ptr;
   size_t copy_size;
   size_t bufsize;
+  off_t old_save_sizeleft;

   status = _flush_write ();
   if (status != record_size && !multi_volume_option)
     archive_write_error (status);
   else
     {
-      records_written++;
+      if ( status > 0 )
+       records_written++;
       bytes_written += status;
     }

@@ -1595,8 +1598,8 @@
   prev_written += bytes_written;
   bytes_written = 0;

-  copy_ptr = record_start->buffer + status;
-  copy_size = buffer_level - status;
+  copy_ptr = record_start->buffer;
+  copy_size = buffer_level + status;
   /* Switch to the next buffer */
   record_index = !record_index;
   init_buffer ();
@@ -1614,6 +1617,7 @@
     add_chunk_header ();
   header = find_next_block ();
   bufsize = available_space_after (header);
+  old_save_sizeleft = save_sizeleft;
   while (bufsize < copy_size)
     {
       memcpy (header->buffer, copy_ptr, bufsize);
@@ -1622,7 +1626,11 @@
       set_next_block_after (header + (bufsize - 1) / BLOCKSIZE);
       header = find_next_block ();
       bufsize = available_space_after (header);
+
+      save_sizeleft = old_save_sizeleft + copy_size;
+      multi_volume_sync();
     }
+  save_sizeleft = old_save_sizeleft;
   memcpy (header->buffer, copy_ptr, copy_size);
   memset (header->buffer + copy_size, 0, bufsize - copy_size);
   set_next_block_after (header + (copy_size - 1) / BLOCKSIZE);