You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by bo...@apache.org on 2017/10/08 16:56:54 UTC

[2/2] commons-compress git commit: COMPRESS-409 remove internal buffering from TarArchiveOutputStream

COMPRESS-409 remove internal buffering from TarArchiveOutputStream


Project: http://git-wip-us.apache.org/repos/asf/commons-compress/repo
Commit: http://git-wip-us.apache.org/repos/asf/commons-compress/commit/c82dc1fc
Tree: http://git-wip-us.apache.org/repos/asf/commons-compress/tree/c82dc1fc
Diff: http://git-wip-us.apache.org/repos/asf/commons-compress/diff/c82dc1fc

Branch: refs/heads/master
Commit: c82dc1fc6238b6f22797c678f10b64c9cd36ddcb
Parents: 77beca1
Author: Stefan Bodewig <bo...@apache.org>
Authored: Sun Oct 8 18:54:22 2017 +0200
Committer: Stefan Bodewig <bo...@apache.org>
Committed: Sun Oct 8 18:56:23 2017 +0200

----------------------------------------------------------------------
 src/changes/changes.xml                         |   4 +
 .../archivers/tar/TarArchiveOutputStream.java   | 112 +++----------------
 .../tar/TarArchiveOutputStreamTest.java         |   4 +
 3 files changed, 22 insertions(+), 98 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/commons-compress/blob/c82dc1fc/src/changes/changes.xml
----------------------------------------------------------------------
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index b29952d..9348e63 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -123,6 +123,10 @@ wanted to create such files.">
         When reading tar headers with name fields containing embedded
         NULs, the name will now be terminated at the first NUL byte.
       </action>
+      <action issue="COMPRESS-409" type="fix" date="2017-10-08">
+        Simplified TarArchiveOutputStream by replacing the internal
+        buffering with new class FixedLengthBlockOutputStream.
+      </action>
     </release>
     <release version="1.14" date="2017-05-14"
              description="Release 1.14">

http://git-wip-us.apache.org/repos/asf/commons-compress/blob/c82dc1fc/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
index 7cff5e3..328b5a9 100644
--- a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
+++ b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java
@@ -34,6 +34,7 @@ import org.apache.commons.compress.archivers.zip.ZipEncoding;
 import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
 import org.apache.commons.compress.utils.CharsetNames;
 import org.apache.commons.compress.utils.CountingOutputStream;
+import org.apache.commons.compress.utils.FixedLengthBlockOutputStream;
 
 /**
  * The TarOutputStream writes a UNIX tar archive as an OutputStream. Methods are provided to put
@@ -83,8 +84,6 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
     private String currName;
     private long currBytes;
     private final byte[] recordBuf;
-    private int assemLen;
-    private final byte[] assemBuf;
     private int longFileMode = LONGFILE_ERROR;
     private int bigNumberMode = BIGNUMBER_ERROR;
     private int recordsWritten;
@@ -102,7 +101,8 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
      */
     private boolean finished = false;
 
-    private final OutputStream out;
+    private final FixedLengthBlockOutputStream out;
+    private final CountingOutputStream countingOut;
 
     private final ZipEncoding zipEncoding;
 
@@ -203,12 +203,11 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
         if (realBlockSize <=0 || realBlockSize % RECORD_SIZE != 0) {
             throw new IllegalArgumentException("Block size must be a multiple of 512 bytes. Attempt to use set size of " + blockSize);
         }
-        out = new CountingOutputStream(os);
+        out = new FixedLengthBlockOutputStream(countingOut = new CountingOutputStream(os),
+                                               RECORD_SIZE);
         this.encoding = encoding;
         this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
 
-        this.assemLen = 0;
-        this.assemBuf = new byte[RECORD_SIZE];
         this.recordBuf = new byte[RECORD_SIZE];
         this.recordsPerBlock = realBlockSize / RECORD_SIZE;
     }
@@ -255,7 +254,7 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
 
     @Override
     public long getBytesWritten() {
-        return ((CountingOutputStream) out).getBytesWritten();
+        return countingOut.getBytesWritten();
     }
 
     /**
@@ -402,32 +401,24 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
         if (!haveUnclosedEntry) {
             throw new IOException("No current entry to close");
         }
-        if (assemLen > 0) {
-            for (int i = assemLen; i < assemBuf.length; ++i) {
-                assemBuf[i] = 0;
-            }
-
-            writeRecord(assemBuf);
-
-            currBytes += assemLen;
-            assemLen = 0;
-        }
-
+        out.flushBlock();
         if (currBytes < currSize) {
             throw new IOException("entry '" + currName + "' closed at '"
                 + currBytes
                 + "' before the '" + currSize
                 + "' bytes specified in the header were written");
         }
+        recordsWritten += (currSize / RECORD_SIZE);
+        if (0 != currSize % RECORD_SIZE) {
+            recordsWritten++;
+        }
         haveUnclosedEntry = false;
     }
 
     /**
      * Writes bytes to the current tar archive entry. This method is aware of the current entry and
      * will throw an exception if you attempt to write bytes past the length specified for the
-     * current entry. The method is also (painfully) aware of the record buffering required by
-     * TarBuffer, and manages buffers that are not a multiple of recordsize in length, including
-     * assembling records from small buffers.
+     * current entry.
      *
      * @param wBuf The buffer to write to the archive.
      * @param wOffset The offset in the buffer from which to get bytes.
@@ -444,63 +435,9 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
                 + "' bytes exceeds size in header of '"
                 + currSize + "' bytes for entry '"
                 + currName + "'");
-
-            //
-            // We have to deal with assembly!!!
-            // The programmer can be writing little 32 byte chunks for all
-            // we know, and we must assemble complete records for writing.
-            // REVIEW Maybe this should be in TarBuffer? Could that help to
-            // eliminate some of the buffer copying.
-            //
-        }
-
-        if (assemLen > 0) {
-            if (assemLen + numToWrite >= recordBuf.length) {
-                final int aLen = recordBuf.length - assemLen;
-
-                System.arraycopy(assemBuf, 0, recordBuf, 0,
-                    assemLen);
-                System.arraycopy(wBuf, wOffset, recordBuf,
-                    assemLen, aLen);
-                writeRecord(recordBuf);
-
-                currBytes += recordBuf.length;
-                wOffset += aLen;
-                numToWrite -= aLen;
-                assemLen = 0;
-            } else {
-                System.arraycopy(wBuf, wOffset, assemBuf, assemLen,
-                    numToWrite);
-
-                wOffset += numToWrite;
-                assemLen += numToWrite;
-                numToWrite = 0;
-            }
-        }
-
-        //
-        // When we get here we have EITHER:
-        // o An empty "assemble" buffer.
-        // o No bytes to write (numToWrite == 0)
-        //
-        while (numToWrite > 0) {
-            if (numToWrite < recordBuf.length) {
-                System.arraycopy(wBuf, wOffset, assemBuf, assemLen,
-                    numToWrite);
-
-                assemLen += numToWrite;
-
-                break;
-            }
-
-            writeRecord(wBuf, wOffset);
-
-            final int num = recordBuf.length;
-
-            currBytes += num;
-            numToWrite -= num;
-            wOffset += num;
         }
+        out.write(wBuf, wOffset, numToWrite);
+        currBytes += numToWrite;
     }
 
     /**
@@ -617,27 +554,6 @@ public class TarArchiveOutputStream extends ArchiveOutputStream {
         recordsWritten++;
     }
 
-    /**
-     * Write an archive record to the archive, where the record may be inside of a larger array
-     * buffer. The buffer must be "offset plus record size" long.
-     *
-     * @param buf The buffer containing the record data to write.
-     * @param offset The offset of the record data within buf.
-     * @throws IOException on error
-     */
-    private void writeRecord(final byte[] buf, final int offset) throws IOException {
-
-        if (offset + RECORD_SIZE > buf.length) {
-            throw new IOException("record has length '" + buf.length
-                + "' with offset '" + offset
-                + "' which is less than the record size of '"
-                + RECORD_SIZE + "'");
-        }
-
-        out.write(buf, offset, RECORD_SIZE);
-        recordsWritten++;
-    }
-
     private void padAsNeeded() throws IOException {
         final int start = recordsWritten % recordsPerBlock;
         if (start != 0) {

http://git-wip-us.apache.org/repos/asf/commons-compress/blob/c82dc1fc/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
index e5f9fbd..3bdfe9d 100644
--- a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
+++ b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java
@@ -659,6 +659,10 @@ public class TarArchiveOutputStreamTest extends AbstractTestCase {
         } catch (IllegalArgumentException e) {
             //expected
         }
+        // test with "content" that is an exact multiple of record length
+        contents = new byte[2048];
+        java.util.Arrays.fill(contents, (byte) 42);
+        testPadding(TarConstants.DEFAULT_BLKSIZE, fileName, contents);
     }
 
     private void testPadding(int blockSize, String fileName, byte[] contents) throws IOException {