You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/07/07 06:21:56 UTC
[GitHub] [commons-compress] theobisproject opened a new pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
theobisproject opened a new pull request #113:
URL: https://github.com/apache/commons-compress/pull/113
Basic idea behind the implementation is to read all headers at instantiation and save the position to the entry data. This will then be accessed as a random access file when reading the entry.
I tried to touch as less exisiting code as possible and only did refactor existing code were I felt it was really needed.
I have the following TODOs left in the code where I think your opinion is needed
- TarArchiveInputStream#canReadEntryData: as noted in the TODO sparse entries are now supported. I see no need for this function any longer and would not include it in the TarFile api. Also we should update the implementation to reflect the now supported sparse entries.
- In the SparseFilesTest extracting of sparsefile-0.1 in pax_gnu_sparse.tar is skipped because of problems with tar on Ubuntu 16.04. I tested this on Ubuntu 18.04 and everything worked fine. Should we reenable extracting of the file?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-683412758
I rebased my changes and included a fix for COMPESS-544 in the TarFile implementation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-727777619
> In the SparseFilesTest extracting of sparsefile-0.1 in pax_gnu_sparse.tar is skipped because of problems with tar on Ubuntu 16.04. I tested this on Ubuntu 18.04 and everything worked fine. Should we reenable extracting of the file?
Ah. I missed this in my last reviews. It's weird. :)
I tested it on Ubuntu 18.04 and it do works now. That test was failed on Ubuntu 16.04 but unfortunly I don't have a Ubuntu 16.04 now. Agree with @kinow that we can leave it as-is and have it in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee merged pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee merged pull request #113:
URL: https://github.com/apache/commons-compress/pull/113
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-755056091
> I removed the code. Now everything I wanted to address is done.
Looks good. I will do some final checks these days.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-750693574
Hey @theobisproject .
Finally I can carve out some time these days. :)
I'm thinking that we can make this PR merged within this Christmas. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-731746841
> `readLastEntry_tarStream` is even slower than eithor
My guess would be that the performance characteristic of `IOUtils.skip` is quite different than the writing to the `NullOutputStream` in the benchmark.
If someone is only interested in the overhead of the `TarFile` implementation this benchmark can be added.
```java
@Benchmark
public void tarFileOverhead() throws IOException {
try (TarFile tarFile = new TarFile(getLargeTarPath())) {
// Do nothing to only measure the overhead of reading all files header information
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654812886
From the sidelines here... I am wondering if the introduction of utility streams would not best be done by depending on Apache Commons IO which already contains a bounded stream for example. What's missing in Commons IO could be added there if general enough.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] bodewig commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
bodewig commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-662513731
Unfortunately I've been unable to do a real review so far. $life is just setting different priorities right now. A few more or less unsorted thoughts:
* I really wish Compress had abstractions that weren't so tightly bound to Java Streams, but we'll likely never find the energy and time to revamp it
* I understand you want to get feedback on whether something like `TarFile` would be accepted rater than the concrete implementation. IMHO this would be a nice addition even though I don't expect many people to require random access to a raw tar archive. Most people likely deal with tar archives that have been compressed and so you'd need to decompress it to a temporary file (or memory) for random access.
* we already have a bunch of things copied from commons-io and our very own BoundedInputStream may even predate the one of commons-io. I wouldn't want to add a new dependency to 1.x unless the reason was better than avoiding dupkiicating a few lines of code.
* have you considered making `TarArchiveEntry` implement `EntryStreamOffsets `?
* I'd be in favour of moving stream implementations to the utils package, in particular if you copy code from the zip package
* nitpicking: the methods you've moved to `TarUtils` probably don't need to be public.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r550947582
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedArchiveInputStream.java
##########
@@ -0,0 +1,98 @@
+/*
Review comment:
I'm +1 for this.
But IIRC there was a issue or mailing discussing about this : the `skip` implemention in Commons IO didn't use the `skip` of the InputStream it's reading from. I will try to find this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/33214895)
Coverage increased (+0.06%) to 87.282% when pulling **bdd25476847d5b347cad288114e46e3ea474fe3e on theobisproject:random-access-tar** into **4eb3bbe8e95f5e3b6388b1e8db28b862cc712294 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/35949060)
Coverage increased (+0.08%) to 87.357% when pulling **ec730d559ed74cb2cfdee3bb5a728fd68b3dbfb6 on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/31902640)
Coverage increased (+0.03%) to 87.316% when pulling **01ea7d3d9fbb629c35f0a20407b8e3975cd07157 on theobisproject:random-access-tar** into **f41756f8024930384a60c5c57eb9eb2913ce1ae0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-730879784
I have tested using your bench on my computer and got this :
```
Benchmark Mode Cnt Score Error Units
ReadLargeTarBenchmark.readAllEntries_tarFile avgt 10 5.601 ± 0.061 s/op
ReadLargeTarBenchmark.readAllEntries_tarStream avgt 10 6.728 ± 0.012 s/op
ReadLargeTarBenchmark.readFirstEntry_tarFile avgt 10 2.279 ± 0.017 s/op
ReadLargeTarBenchmark.readFirstEntry_tarStream avgt 10 0.001 ± 0.001 s/op
ReadLargeTarBenchmark.readLastEntry_tarFile avgt 10 2.266 ± 0.020 s/op
ReadLargeTarBenchmark.readLastEntry_tarStream avgt 10 13.068 ± 0.030 s/op
ReadLargeTarBenchmark.readSecondEntry_tarFile avgt 10 2.257 ± 0.012 s/op
ReadLargeTarBenchmark.readSecondEntry_tarStream avgt 10 0.001 ± 0.001 s/op
```
The score is average time by setting `BenchmarkMode` to `Mode.AverageTime`. I'm testing with linux-4.4.tar, which has a size of 618MB with 55708 entries. I'm testing on my machine : Windows10, SSD, Java 8.
The result is interesting. Seems the `tarFile` needs an extra 2.65s to read all entries. `readAllEntries_tarStream` is slower than `readAllEntries_tarFile`, and `readLastEntry_tarStream` is even slower than eithor. Will try to figure out what's going on here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/36067610)
Coverage increased (+0.1%) to 87.376% when pulling **f555ad9b8e6f34a9b8a67cb5f28e9b33276f7763 on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-752295265
> I don't think we need this method because the TarFile#getInputStream only takes an instance of TarArchiveEntry.
+1.
The `canReadEntryData` in `TarArchiveInputStream` was checking if the entry is a sparse entry or not - cause the sparse entries were not supported. The sparse entry was supported later but we could not remove the `canReadEntryData` because it's a API breaking change.
I don't think we need it in `TarFile` any more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-678318459
@dependabot rebase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-730641999
That's still a good approach for users that are aware of the weakness of the approach @theobisproject . Your performance test could be incorporated in commons-compress with JMH too, disabled by default. This way in case developers need to work on these classes, they may re-run the test to check for regressions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/32297922)
Coverage increased (+0.007%) to 87.295% when pulling **1e6b0bd66b2c95196614bc14fe3cef1478c458b8 on theobisproject:random-access-tar** into **f41756f8024930384a60c5c57eb9eb2913ce1ae0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548909995
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedNIOInputStream.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * NIO backed bounded input stream for reading a predefined amount of data from.
+ * @since 1.21
+ */
+public abstract class BoundedNIOInputStream extends InputStream {
Review comment:
Since the `BoundedInputStream` class is still there how about naming it `BoundedArchiveInputStream`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/35946070)
Coverage increased (+0.1%) to 87.373% when pulling **0567a3aebf065cd76d52111fbf30efcca9d7e16b on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r551404112
##########
File path: src/test/java/org/apache/commons/compress/archivers/TarTestCase.java
##########
@@ -335,6 +482,18 @@ public void testCOMPRESS178() throws Exception {
in.close();
}
+ @Test
+ public void testTarFileCOMPRESS178() throws Exception {
+ final File input = getFile("COMPRESS-178.tar");
+ try (final TarFile tarFile = new TarFile(input)) {
+ // Compared to the TarArchiveInputStream all entries are read when instantiating the tar file
+ fail("Expected IOException");
+ } catch (final IOException e) {
Review comment:
`assertThrows` is not available in Junit 4. The test is equivalent to the already exisiting test for the `TarArchiveInputStream`. Should I change both tests to use the `expected` property in the `@Test` annotation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-727703377
Hi @theobisproject
I have started a thread in mailing list about this.
> Since I will have to do quite some work with the removal of code duplication I'd like to know how long you are willing to wait for feedback.
I'm hoping to have this change dealed within version 1.21.
Thanks for your great work once again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-751975974
And the japicmp report no BC breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-730065042
> My testing showed the TarFile and TarArchiveStream performance is equivalent for sequential access. The performance gain for accessing a single entry in the TarFile is huge because the TarArchiveStream needs to read and skip all data until it reaches the entry.
> If you would like some numbers for some cases feel free to name them. I can create some benchmarks and share them with you.
That's interesting. Would you please upload the tests so I can have a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-751144813
> So we should decide what we want to do about it in this PR.
Let's start with the easiest one : there are some conflicts now. Could you resolve them? :)
BTW : wish you a great christmas. :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-662962115
I know you are working on this library in your free time. So I did expect this big change to take some time before it is integrated.
> IMHO this would be a nice addition even though I don't expect many people to require random access to a raw tar archive. Most people likely deal with tar archives that have been compressed and so you'd need to decompress it to a temporary file (or memory) for random access.
As you pointed out this only brings better speed for some special cases e.g. random access or somthing like the `TarLister` where no content is read and therefore the stream needs to read and discard all data.
Specifially for compressed archives it can even take longer depending on the target where the file is decompressed to.
> have you considered making `TarArchiveEntry` implement `EntryStreamOffsets `?
Seems like I missed this interface. Since this exactly what I wanted to implement I will change the implementation to implement the `EntryStreamOffsets` interface.
> I'd be in favour of moving stream implementations to the utils package, in particular if you copy code from the zip package
The code I moved out of the `ZipFile` class is already located in the utils package. Only the Tar specific streams I didn't create in the utils package because in my opinion nobody outside the library should access them and currently no other archive needs similar streams.
> nitpicking: the methods you've moved to `TarUtils` probably don't need to be public.
I agree and will change the code which was moved to the `TarUtils` to be not public.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r550266500
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedArchiveInputStream.java
##########
@@ -0,0 +1,98 @@
+/*
Review comment:
Is it time to depend on Commons IO instead of creating a mini Commons IO in Compress?
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
##########
@@ -553,6 +554,23 @@ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final
parseTarHeader(headerBuf, encoding, false, lenient);
}
+ /**
+ * Construct an entry from an archive's header bytes for random access tar. File is set to null.
+ * @param headerBuf the header bytes from a tar archive entry.
+ * @param encoding encoding to use for file names.
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link #UNKNOWN}. When set to false such illegal fields cause an exception instead.
+ * @param dataOffset position of the entry data in the random access file.
+ * @since 1.21
+ * @throws IllegalArgumentException if any of the numeric fields have an invalid format.
+ * @throws IOException on error.
+ */
+ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final boolean lenient,
+ final long dataOffset) throws IOException {
+ this(headerBuf, encoding, lenient);
+ this.dataOffset = dataOffset;
+ }
Review comment:
Should we throw an IAE if the offset is negative or larger than the parent can hold?
##########
File path: src/test/java/org/apache/commons/compress/archivers/TarTestCase.java
##########
@@ -335,6 +482,18 @@ public void testCOMPRESS178() throws Exception {
in.close();
}
+ @Test
+ public void testTarFileCOMPRESS178() throws Exception {
+ final File input = getFile("COMPRESS-178.tar");
+ try (final TarFile tarFile = new TarFile(input)) {
+ // Compared to the TarArchiveInputStream all entries are read when instantiating the tar file
+ fail("Expected IOException");
+ } catch (final IOException e) {
Review comment:
Use assertThrows().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/36008631)
Coverage increased (+0.08%) to 87.357% when pulling **37a049c206d39f42db64b20ec42cba8891431e86 on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-752253262
I've gone through the changes once more and noticed the changes from COMPRESS-559 is missing for the sparse entry test with `TarFile`.
> Are there any other work left now? @theobisproject
I'd like to hear your opinion about my TODO comments in the `SparseFilesTest`. I left out the `canReadEntryData` method which the `TarArchiveInputStream` has but kept the checks in my added tests. I don't think we need this method because the `TarFile#getInputStream` only takes an instance of `TarArchiveEntry`. Should I add this method or just remove the commented out code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-729799712
@kinow Thank you for your feedback!
> > In the SparseFilesTest extracting of sparsefile-0.1 in pax_gnu_sparse.tar is skipped because of problems with tar on Ubuntu 16.04. I tested this on Ubuntu 18.04 and everything worked fine. Should we reenable extracting of the file?
>
> +1, if that works with newer versions. Would be good to leave a NOTE/comment somewhere though, in case the test fails again. Even better if we were able to pinpoint what broke it in Ubuntu 16.04 (but can be done later 👍 )
>
> To keep this PR simpler, we can leave as-is, and create a new JIRA to follow-up on this.
> Ah. I missed this in my last reviews. It's weird. :)
> I tested it on Ubuntu 18.04 and it do works now. That test was failed on Ubuntu 16.04 but unfortunly I don't have a Ubuntu 16.04 now. Agree with @kinow that we can leave it as-is and have it in a separate PR.
I can create a new JIRA and fix this on master next weekend if nothing get's in my way.
> @theobisproject , @PeterAlfredLee , my only other comment is about the name `TarFile`. Is there any risk in using this class name, with a feature that is not part of the tar format (I don't know the format well enough, but I think you mentioned it in your e-mail to the mailing list Lee?)?
>
> I mean, could it be that in the future we may need to create a `TarFile`, that doesn't/cannot support the random read, and in which case we would have to find another name?
I don't think there is any risk even that the random access is not supported by the format but emulated by the implementation. You still can use the `TarFile` class to read the content sequentially since the order of the entries is preserved. Also this naming would be consistent with the already exisiting `ZipFile` implementation.
From a performance point of view the overhead needed to allow the random access is as small as possible since only the headers of the entries are read (excluding some special cases with e.g. long filenames where the information is stored in a different entry) and all file data is jumped over by setting the position in the file to the start of the next header. My testing showed the `TarFile` and `TarArchiveStream` performance is equivalent for sequential access. The performance gain for accessing a single entry in the `TarFile` is huge because the `TarArchiveStream` needs to read and skip all data until it reaches the entry.
If you would like some numbers for some cases feel free to name them. I can create some benchmarks and share them with you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-750693574
Hey @theobisproject .
Finally I can carve out some time these days. :(
I'm thinking that we can make this PR merged within this Christmas. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-757223321
Thank you all for taking the time to review this change!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548929154
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
+
+ private static final int SMALL_BUFFER_SIZE = 256;
+
+ private final byte[] smallBuf = new byte[SMALL_BUFFER_SIZE];
+
+ private final SeekableByteChannel archive;
+
+ /**
+ * The encoding of the tar file
+ */
+ private final ZipEncoding zipEncoding;
+
+ private final LinkedList<TarArchiveEntry> entries = new LinkedList<>();
+
+ private final int blockSize;
+
+ private final boolean lenient;
+
+ private final int recordSize;
+
+ private final ByteBuffer recordBuffer;
+
+ // the global sparse headers, this is only used in PAX Format 0.X
+ private final List<TarArchiveStructSparse> globalSparseHeaders = new ArrayList<>();
+
+ private boolean hasHitEOF;
+
+ /**
+ * The meta-data about the current entry
+ */
+ private TarArchiveEntry currEntry;
+
+ // the global PAX header
+ private Map<String, String> globalPaxHeaders = new HashMap<>();
+
+ private final Map<String, List<InputStream>> sparseInputStreams = new HashMap<>();
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final String encoding) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final boolean lenient) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive) throws IOException {
+ this(archive.toPath());
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final String encoding) throws IOException {
+ this(archive.toPath(), encoding);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final boolean lenient) throws IOException {
+ this(archive.toPath(), lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final String encoding) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final boolean lenient) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the seekable byte channel to use
+ * @param blockSize the blocks size to use
+ * @param recordSize the record size to use
+ * @param encoding the encoding to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final SeekableByteChannel archive, final int blockSize, final int recordSize, final String encoding, final boolean lenient) throws IOException {
+ this.archive = archive;
+ this.hasHitEOF = false;
+ this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
+ this.recordSize = recordSize;
+ this.recordBuffer = ByteBuffer.allocate(this.recordSize);
+ this.blockSize = blockSize;
+ this.lenient = lenient;
+
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive. This will skip
+ * to the end of the current entry, if there is one, and
+ * place the position of the channel at the header of the
+ * next entry, and read the header and instantiate a new
+ * TarEntry from the header bytes and return that entry.
+ * If there are no more entries in the archive, null will
+ * be returned to indicate that the end of the archive has
+ * been reached.
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private TarArchiveEntry getNextTarEntry() throws IOException {
+ if (isAtEOF()) {
+ return null;
+ }
+
+ if (currEntry != null) {
+ // Skip to the end of the entry
+ archive.position(currEntry.getDataOffset() + currEntry.getSize());
+ throwExceptionIfPositionIsNotInArchive();
+
+ skipRecordPadding();
+ }
+
+ final ByteBuffer headerBuf = getRecord();
+ if (null == headerBuf) {
+ /* hit EOF */
+ currEntry = null;
+ return null;
+ }
+
+ try {
+ currEntry = new TarArchiveEntry(headerBuf.array(), zipEncoding, lenient, archive.position());
+ } catch (final IllegalArgumentException e) {
+ throw new IOException("Error detected parsing the header", e);
+ }
+
+ if (currEntry.isGNULongLinkEntry()) {
+ final byte[] longLinkData = getLongNameData();
+ if (longLinkData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long link entry name not followed by
+ // entry
+ return null;
+ }
+ currEntry.setLinkName(zipEncoding.decode(longLinkData));
+ }
+
+ if (currEntry.isGNULongNameEntry()) {
+ final byte[] longNameData = getLongNameData();
+ if (longNameData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by
+ // entry
+ return null;
+ }
+
+ // COMPRESS-509 : the name of directories should end with '/'
+ final String name = zipEncoding.decode(longNameData);
+ currEntry.setName(name);
+ if (currEntry.isDirectory() && !name.endsWith("/")) {
+ currEntry.setName(name + "/");
+ }
+ }
+
+ if (currEntry.isGlobalPaxHeader()) { // Process Global Pax headers
+ readGlobalPaxHeaders();
+ }
+
+ try {
+ if (currEntry.isPaxHeader()) { // Process Pax headers
+ paxHeaders();
+ } else if (!globalPaxHeaders.isEmpty()) {
+ applyPaxHeadersToCurrentEntry(globalPaxHeaders, globalSparseHeaders);
+ }
+ } catch (NumberFormatException e) {
+ throw new IOException("Error detected parsing the pax header", e);
+ }
+
+ if (currEntry.isOldGNUSparse()) { // Process sparse files
+ readOldGNUSparse();
+ }
+
+ return currEntry;
+ }
+
+ /**
+ * Adds the sparse chunks from the current entry to the sparse chunks,
+ * including any additional sparse entries following the current entry.
+ *
+ * @throws IOException when reading the sparse entry fails
+ */
+ private void readOldGNUSparse() throws IOException {
+ if (currEntry.isExtended()) {
+ TarArchiveSparseEntry entry;
+ do {
+ final ByteBuffer headerBuf = getRecord();
+ if (headerBuf == null) {
+ currEntry = null;
+ break;
+ }
+ entry = new TarArchiveSparseEntry(headerBuf.array());
+ currEntry.getSparseHeaders().addAll(entry.getSparseHeaders());
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ } while (entry.isExtended());
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ /**
+ * Build the input streams consisting of all-zero input streams and non-zero input streams.
+ * When reading from the non-zero input streams, the data is actually read from the original input stream.
+ * The size of each input stream is introduced by the sparse headers.
+ *
+ * @implNote Some all-zero input streams and non-zero input streams have the size of 0. We DO NOT store the
+ * 0 size input streams because they are meaningless.
+ */
+ private void buildSparseInputStreams() throws IOException {
+ List<InputStream> streams = new ArrayList<>();
+
+ final List<TarArchiveStructSparse> sparseHeaders = currEntry.getSparseHeaders();
+ // sort the sparse headers in case they are written in wrong order
+ if (sparseHeaders != null && sparseHeaders.size() > 1) {
+ final Comparator<TarArchiveStructSparse> sparseHeaderComparator = new Comparator<TarArchiveStructSparse>() {
+ @Override
+ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse q) {
+ Long pOffset = p.getOffset();
+ Long qOffset = q.getOffset();
+ return pOffset.compareTo(qOffset);
+ }
+ };
+ Collections.sort(sparseHeaders, sparseHeaderComparator);
+ }
+
+ if (sparseHeaders != null) {
+ // Stream doesn't need to be closed at all as it doesn't use any resources
+ final InputStream zeroInputStream = new TarArchiveSparseZeroInputStream(); //NOSONAR
+ long offset = 0;
+ long numberOfZeroBytesInSparseEntry = 0;
+ for (TarArchiveStructSparse sparseHeader : sparseHeaders) {
+ if (sparseHeader.getOffset() == 0 && sparseHeader.getNumbytes() == 0) {
+ break;
+ }
+
+ if ((sparseHeader.getOffset() - offset) < 0) {
+ throw new IOException("Corrupted struct sparse detected");
+ }
+
+ // only store the input streams with non-zero size
+ if ((sparseHeader.getOffset() - offset) > 0) {
+ long sizeOfZeroByteStream = sparseHeader.getOffset() - offset;
+ streams.add(new BoundedInputStream(zeroInputStream, sizeOfZeroByteStream));
+ numberOfZeroBytesInSparseEntry += sizeOfZeroByteStream;
+ }
+
+ // only store the input streams with non-zero size
+ if (sparseHeader.getNumbytes() > 0) {
+ long start =
+ currEntry.getDataOffset() + sparseHeader.getOffset() - numberOfZeroBytesInSparseEntry;
+ streams.add(new BoundedSeekableByteChannelInputStream(start, sparseHeader.getNumbytes(), archive));
+ }
+
+ offset = sparseHeader.getOffset() + sparseHeader.getNumbytes();
+ }
+ }
+
+ sparseInputStreams.put(currEntry.getName(), streams);
+ }
+
+ /**
+ * Update the current entry with the read pax headers
+ * @param headers Headers read from the pax header
+ * @param sparseHeaders Sparse headers read from pax header
+ */
+ private void applyPaxHeadersToCurrentEntry(final Map<String, String> headers, final List<TarArchiveStructSparse> sparseHeaders) {
+ currEntry.updateEntryFromPaxHeaders(headers);
+ currEntry.setSparseHeaders(sparseHeaders);
+ }
+
+ /**
+ * <p>
+ * For PAX Format 0.0, the sparse headers(GNU.sparse.offset and GNU.sparse.numbytes)
+ * may appear multi times, and they look like:
+ * <pre>
+ * GNU.sparse.size=size
+ * GNU.sparse.numblocks=numblocks
+ * repeat numblocks times
+ * GNU.sparse.offset=offset
+ * GNU.sparse.numbytes=numbytes
+ * end repeat
+ * </pre>
+ *
+ * <p>
+ * For PAX Format 0.1, the sparse headers are stored in a single variable : GNU.sparse.map
+ * <pre>
+ * GNU.sparse.map
+ * Map of non-null data chunks. It is a string consisting of comma-separated values "offset,size[,offset-1,size-1...]"
+ * </pre>
+ *
+ * <p>
+ * For PAX Format 1.X:
+ * <br>
+ * The sparse map itself is stored in the file data block, preceding the actual file data.
+ * It consists of a series of decimal numbers delimited by newlines. The map is padded with nulls to the nearest block boundary.
+ * The first number gives the number of entries in the map. Following are map entries, each one consisting of two numbers
+ * giving the offset and size of the data block it describes.
+ * @throws IOException
+ */
+ private void paxHeaders() throws IOException {
+ List<TarArchiveStructSparse> sparseHeaders = new ArrayList<>();
+ final Map<String, String> headers;
+ try (final InputStream input = getInputStream(currEntry)) {
+ headers = TarUtils.parsePaxHeaders(input, sparseHeaders, globalPaxHeaders);
+ }
+
+ // for 0.1 PAX Headers
+ if (headers.containsKey("GNU.sparse.map")) {
+ sparseHeaders = TarUtils.parsePAX01SparseHeaders(headers.get("GNU.sparse.map"));
+ }
+ getNextTarEntry(); // Get the actual file entry
+ if (currEntry == null) {
+ throw new IOException("premature end of tar archive. Didn't find any entry after PAX header.");
+ }
+ applyPaxHeadersToCurrentEntry(headers, sparseHeaders);
+
+ // for 1.0 PAX Format, the sparse map is stored in the file data block
+ if (currEntry.isPaxGNU1XSparse()) {
+ try (final InputStream input = getInputStream(currEntry)) {
+ sparseHeaders = TarUtils.parsePAX1XSparseHeaders(input, recordSize);
+ }
+ currEntry.setSparseHeaders(sparseHeaders);
+ // data of the entry is after the pax gnu entry. So we need to update the data position once again
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ private void readGlobalPaxHeaders() throws IOException {
+ try (InputStream input = getInputStream(currEntry)) {
+ globalPaxHeaders = TarUtils.parsePaxHeaders(input, globalSparseHeaders, globalPaxHeaders);
+ }
+ getNextTarEntry(); // Get the actual file entry
+
+ if (currEntry == null) {
+ throw new IOException("Error detected parsing the pax header");
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive as longname data.
+ *
+ * @return The next entry in the archive as longname data, or null.
+ * @throws IOException on error
+ */
+ private byte[] getLongNameData() throws IOException {
+ final ByteArrayOutputStream longName = new ByteArrayOutputStream();
+ int length;
+ try (final InputStream in = getInputStream(currEntry)) {
+ while ((length = in.read(smallBuf)) >= 0) {
+ longName.write(smallBuf, 0, length);
+ }
+ }
+ getNextTarEntry();
+ if (currEntry == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by entry
+ return null;
+ }
+ byte[] longNameData = longName.toByteArray();
+ // remove trailing null terminator(s)
+ length = longNameData.length;
+ while (length > 0 && longNameData[length - 1] == 0) {
+ --length;
+ }
+ if (length != longNameData.length) {
+ final byte[] l = new byte[length];
+ System.arraycopy(longNameData, 0, l, 0, length);
+ longNameData = l;
+ }
+ return longNameData;
+ }
+
+ /**
+ * The last record block should be written at the full size, so skip any
+ * additional space used to fill a record after an entry
+ *
+ * @throws IOException when skipping the padding of the record fails
+ */
+ private void skipRecordPadding() throws IOException {
+ if (!isDirectory() && currEntry.getSize() > 0 && currEntry.getSize() % recordSize != 0) {
+ final long numRecords = (currEntry.getSize() / recordSize) + 1;
+ final long padding = (numRecords * recordSize) - currEntry.getSize();
+ archive.position(archive.position() + padding);
+ throwExceptionIfPositionIsNotInArchive();
+ }
+ }
+
+ /**
+ * Checks if the current position of the SeekableByteChannel is in the archive.
+ * @throws IOException If the position is not in the archive
+ */
+ private void throwExceptionIfPositionIsNotInArchive() throws IOException {
+ if (archive.size() < archive.position()) {
+ throw new IOException("Truncated TAR archive");
+ }
+ }
+
+ /**
+ * Get the next record in this tar archive. This will skip
+ * over any remaining data in the current entry, if there
+ * is one, and place the input stream at the header of the
+ * next entry.
+ *
+ * <p>If there are no more entries in the archive, null will be
+ * returned to indicate that the end of the archive has been
+ * reached. At the same time the {@code hasHitEOF} marker will be
+ * set to true.</p>
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private ByteBuffer getRecord() throws IOException {
+ ByteBuffer headerBuf = readRecord();
+ setAtEOF(isEOFRecord(headerBuf));
+ if (isAtEOF() && headerBuf != null) {
+ // Consume rest
+ tryToConsumeSecondEOFRecord();
+ // TODO: This is present in the TarArchiveInputStream but I don't know if we need this in the random access implementation. All tests are passing ...
+ // consumeRemainderOfLastBlock();
Review comment:
I think we should leave the `consumeRemainderOfLastBlock` as it was.
According to the comment :
```
/**
* This method is invoked once the end of the archive is hit, it
* tries to consume the remaining bytes under the assumption that
* the tool creating this archive has padded the last block.
*/
```
This is used to deal with the last block with padding zero. Even through some tar implementions don't implement this, this is part of tar specifiction.
See also [this](https://www.gnu.org/software/tar/manual/html_node/Standard.html) :
> When writing an archive, the last record of blocks should be written at the full size, with blocks after the zero block containing all zeros.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/34078445)
Coverage increased (+0.06%) to 87.274% when pulling **065929324450f40a64314703a06b80a6317cde73 on theobisproject:random-access-tar** into **555daa4e9bfca0df3449f5484d5b87a5194a5abd on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548976007
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,736 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
+
+ private static final int SMALL_BUFFER_SIZE = 256;
+
+ private final byte[] smallBuf = new byte[SMALL_BUFFER_SIZE];
+
+ private final SeekableByteChannel archive;
+
+ /**
+ * The encoding of the tar file
+ */
+ private final ZipEncoding zipEncoding;
+
+ private final LinkedList<TarArchiveEntry> entries = new LinkedList<>();
+
+ private final int blockSize;
+
+ private final boolean lenient;
+
+ private final int recordSize;
+
+ private final ByteBuffer recordBuffer;
+
+ // the global sparse headers, this is only used in PAX Format 0.X
+ private final List<TarArchiveStructSparse> globalSparseHeaders = new ArrayList<>();
+
+ private boolean hasHitEOF;
+
+ /**
+ * The meta-data about the current entry
+ */
+ private TarArchiveEntry currEntry;
+
+ // the global PAX header
+ private Map<String, String> globalPaxHeaders = new HashMap<>();
+
+ private final Map<String, List<InputStream>> sparseInputStreams = new HashMap<>();
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final String encoding) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final boolean lenient) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive) throws IOException {
+ this(archive.toPath());
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final String encoding) throws IOException {
+ this(archive.toPath(), encoding);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final boolean lenient) throws IOException {
+ this(archive.toPath(), lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final String encoding) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final boolean lenient) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the seekable byte channel to use
+ * @param blockSize the blocks size to use
+ * @param recordSize the record size to use
+ * @param encoding the encoding to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final SeekableByteChannel archive, final int blockSize, final int recordSize, final String encoding, final boolean lenient) throws IOException {
+ this.archive = archive;
+ this.hasHitEOF = false;
+ this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
+ this.recordSize = recordSize;
+ this.recordBuffer = ByteBuffer.allocate(this.recordSize);
+ this.blockSize = blockSize;
+ this.lenient = lenient;
+
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive. This will skip
+ * to the end of the current entry, if there is one, and
+ * place the position of the channel at the header of the
+ * next entry, and read the header and instantiate a new
+ * TarEntry from the header bytes and return that entry.
+ * If there are no more entries in the archive, null will
+ * be returned to indicate that the end of the archive has
+ * been reached.
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private TarArchiveEntry getNextTarEntry() throws IOException {
+ if (isAtEOF()) {
+ return null;
+ }
+
+ if (currEntry != null) {
+ // Skip to the end of the entry
+ archive.position(currEntry.getDataOffset() + currEntry.getSize());
+ throwExceptionIfPositionIsNotInArchive();
+
+ skipRecordPadding();
+ }
+
+ final ByteBuffer headerBuf = getRecord();
+ if (null == headerBuf) {
+ /* hit EOF */
+ currEntry = null;
+ return null;
+ }
+
+ try {
+ currEntry = new TarArchiveEntry(headerBuf.array(), zipEncoding, lenient, archive.position());
+ } catch (final IllegalArgumentException e) {
+ throw new IOException("Error detected parsing the header", e);
+ }
+
+ if (currEntry.isGNULongLinkEntry()) {
+ final byte[] longLinkData = getLongNameData();
+ if (longLinkData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long link entry name not followed by
+ // entry
+ return null;
+ }
+ currEntry.setLinkName(zipEncoding.decode(longLinkData));
+ }
+
+ if (currEntry.isGNULongNameEntry()) {
+ final byte[] longNameData = getLongNameData();
+ if (longNameData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by
+ // entry
+ return null;
+ }
+
+ // COMPRESS-509 : the name of directories should end with '/'
+ final String name = zipEncoding.decode(longNameData);
+ currEntry.setName(name);
+ if (currEntry.isDirectory() && !name.endsWith("/")) {
+ currEntry.setName(name + "/");
+ }
+ }
+
+ if (currEntry.isGlobalPaxHeader()) { // Process Global Pax headers
+ readGlobalPaxHeaders();
+ }
+
+ try {
+ if (currEntry.isPaxHeader()) { // Process Pax headers
+ paxHeaders();
+ } else if (!globalPaxHeaders.isEmpty()) {
+ applyPaxHeadersToCurrentEntry(globalPaxHeaders, globalSparseHeaders);
+ }
+ } catch (NumberFormatException e) {
+ throw new IOException("Error detected parsing the pax header", e);
+ }
+
+ if (currEntry.isOldGNUSparse()) { // Process sparse files
+ readOldGNUSparse();
+ }
+
+ return currEntry;
+ }
+
+ /**
+ * Adds the sparse chunks from the current entry to the sparse chunks,
+ * including any additional sparse entries following the current entry.
+ *
+ * @throws IOException when reading the sparse entry fails
+ */
+ private void readOldGNUSparse() throws IOException {
+ if (currEntry.isExtended()) {
+ TarArchiveSparseEntry entry;
+ do {
+ final ByteBuffer headerBuf = getRecord();
+ if (headerBuf == null) {
+ currEntry = null;
+ break;
+ }
+ entry = new TarArchiveSparseEntry(headerBuf.array());
+ currEntry.getSparseHeaders().addAll(entry.getSparseHeaders());
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ } while (entry.isExtended());
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ /**
+ * Build the input streams consisting of all-zero input streams and non-zero input streams.
+ * When reading from the non-zero input streams, the data is actually read from the original input stream.
+ * The size of each input stream is introduced by the sparse headers.
+ *
+ * @implNote Some all-zero input streams and non-zero input streams have the size of 0. We DO NOT store the
+ * 0 size input streams because they are meaningless.
+ */
+ private void buildSparseInputStreams() throws IOException {
+ List<InputStream> streams = new ArrayList<>();
+
+ final List<TarArchiveStructSparse> sparseHeaders = currEntry.getSparseHeaders();
+ // sort the sparse headers in case they are written in wrong order
+ if (sparseHeaders != null && sparseHeaders.size() > 1) {
+ final Comparator<TarArchiveStructSparse> sparseHeaderComparator = new Comparator<TarArchiveStructSparse>() {
+ @Override
+ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse q) {
+ Long pOffset = p.getOffset();
+ Long qOffset = q.getOffset();
+ return pOffset.compareTo(qOffset);
+ }
+ };
+ Collections.sort(sparseHeaders, sparseHeaderComparator);
+ }
+
+ if (sparseHeaders != null) {
+ // Stream doesn't need to be closed at all as it doesn't use any resources
+ final InputStream zeroInputStream = new TarArchiveSparseZeroInputStream(); //NOSONAR
+ long offset = 0;
+ long numberOfZeroBytesInSparseEntry = 0;
+ for (TarArchiveStructSparse sparseHeader : sparseHeaders) {
+ if (sparseHeader.getOffset() == 0 && sparseHeader.getNumbytes() == 0) {
+ break;
+ }
+
+ if ((sparseHeader.getOffset() - offset) < 0) {
+ throw new IOException("Corrupted struct sparse detected");
+ }
+
+ // only store the input streams with non-zero size
+ if ((sparseHeader.getOffset() - offset) > 0) {
+ long sizeOfZeroByteStream = sparseHeader.getOffset() - offset;
+ streams.add(new BoundedInputStream(zeroInputStream, sizeOfZeroByteStream));
+ numberOfZeroBytesInSparseEntry += sizeOfZeroByteStream;
+ }
+
+ // only store the input streams with non-zero size
+ if (sparseHeader.getNumbytes() > 0) {
+ long start =
+ currEntry.getDataOffset() + sparseHeader.getOffset() - numberOfZeroBytesInSparseEntry;
+ streams.add(new BoundedSeekableByteChannelInputStream(start, sparseHeader.getNumbytes(), archive));
+ }
+
+ offset = sparseHeader.getOffset() + sparseHeader.getNumbytes();
+ }
+ }
+
+ sparseInputStreams.put(currEntry.getName(), streams);
+ }
+
+ /**
+ * Update the current entry with the read pax headers
+ * @param headers Headers read from the pax header
+ * @param sparseHeaders Sparse headers read from pax header
+ */
+ private void applyPaxHeadersToCurrentEntry(final Map<String, String> headers, final List<TarArchiveStructSparse> sparseHeaders) {
+ currEntry.updateEntryFromPaxHeaders(headers);
+ currEntry.setSparseHeaders(sparseHeaders);
+ }
+
+ /**
+ * <p>
+ * For PAX Format 0.0, the sparse headers(GNU.sparse.offset and GNU.sparse.numbytes)
+ * may appear multi times, and they look like:
+ * <pre>
+ * GNU.sparse.size=size
+ * GNU.sparse.numblocks=numblocks
+ * repeat numblocks times
+ * GNU.sparse.offset=offset
+ * GNU.sparse.numbytes=numbytes
+ * end repeat
+ * </pre>
+ *
+ * <p>
+ * For PAX Format 0.1, the sparse headers are stored in a single variable : GNU.sparse.map
+ * <pre>
+ * GNU.sparse.map
+ * Map of non-null data chunks. It is a string consisting of comma-separated values "offset,size[,offset-1,size-1...]"
+ * </pre>
+ *
+ * <p>
+ * For PAX Format 1.X:
+ * <br>
+ * The sparse map itself is stored in the file data block, preceding the actual file data.
+ * It consists of a series of decimal numbers delimited by newlines. The map is padded with nulls to the nearest block boundary.
+ * The first number gives the number of entries in the map. Following are map entries, each one consisting of two numbers
+ * giving the offset and size of the data block it describes.
+ * @throws IOException
+ */
+ private void paxHeaders() throws IOException {
+ List<TarArchiveStructSparse> sparseHeaders = new ArrayList<>();
+ final Map<String, String> headers;
+ try (final InputStream input = getInputStream(currEntry)) {
+ headers = TarUtils.parsePaxHeaders(input, sparseHeaders, globalPaxHeaders);
+ }
+
+ // for 0.1 PAX Headers
+ if (headers.containsKey("GNU.sparse.map")) {
+ sparseHeaders = TarUtils.parsePAX01SparseHeaders(headers.get("GNU.sparse.map"));
+ }
+ getNextTarEntry(); // Get the actual file entry
+ if (currEntry == null) {
+ throw new IOException("premature end of tar archive. Didn't find any entry after PAX header.");
+ }
+ applyPaxHeadersToCurrentEntry(headers, sparseHeaders);
+
+ // for 1.0 PAX Format, the sparse map is stored in the file data block
+ if (currEntry.isPaxGNU1XSparse()) {
+ try (final InputStream input = getInputStream(currEntry)) {
+ sparseHeaders = TarUtils.parsePAX1XSparseHeaders(input, recordSize);
+ }
+ currEntry.setSparseHeaders(sparseHeaders);
+ // data of the entry is after the pax gnu entry. So we need to update the data position once again
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ private void readGlobalPaxHeaders() throws IOException {
+ try (InputStream input = getInputStream(currEntry)) {
+ globalPaxHeaders = TarUtils.parsePaxHeaders(input, globalSparseHeaders, globalPaxHeaders);
+ }
+ getNextTarEntry(); // Get the actual file entry
+
+ if (currEntry == null) {
+ throw new IOException("Error detected parsing the pax header");
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive as longname data.
+ *
+ * @return The next entry in the archive as longname data, or null.
+ * @throws IOException on error
+ */
+ private byte[] getLongNameData() throws IOException {
+ final ByteArrayOutputStream longName = new ByteArrayOutputStream();
+ int length;
+ try (final InputStream in = getInputStream(currEntry)) {
+ while ((length = in.read(smallBuf)) >= 0) {
+ longName.write(smallBuf, 0, length);
+ }
+ }
+ getNextTarEntry();
+ if (currEntry == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by entry
+ return null;
+ }
+ byte[] longNameData = longName.toByteArray();
+ // remove trailing null terminator(s)
+ length = longNameData.length;
+ while (length > 0 && longNameData[length - 1] == 0) {
+ --length;
+ }
+ if (length != longNameData.length) {
+ final byte[] l = new byte[length];
+ System.arraycopy(longNameData, 0, l, 0, length);
+ longNameData = l;
+ }
+ return longNameData;
+ }
+
+ /**
+ * The last record block should be written at the full size, so skip any
+ * additional space used to fill a record after an entry
+ *
+ * @throws IOException when skipping the padding of the record fails
+ */
+ private void skipRecordPadding() throws IOException {
+ if (!isDirectory() && currEntry.getSize() > 0 && currEntry.getSize() % recordSize != 0) {
+ final long numRecords = (currEntry.getSize() / recordSize) + 1;
+ final long padding = (numRecords * recordSize) - currEntry.getSize();
+ archive.position(archive.position() + padding);
+ throwExceptionIfPositionIsNotInArchive();
+ }
+ }
+
+ /**
+ * Checks if the current position of the SeekableByteChannel is in the archive.
+ * @throws IOException If the position is not in the archive
+ */
+ private void throwExceptionIfPositionIsNotInArchive() throws IOException {
+ if (archive.size() < archive.position()) {
+ throw new IOException("Truncated TAR archive");
+ }
+ }
+
+ /**
+ * Get the next record in this tar archive. This will skip
+ * over any remaining data in the current entry, if there
+ * is one, and place the input stream at the header of the
+ * next entry.
+ *
+ * <p>If there are no more entries in the archive, null will be
+ * returned to indicate that the end of the archive has been
+ * reached. At the same time the {@code hasHitEOF} marker will be
+ * set to true.</p>
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private ByteBuffer getRecord() throws IOException {
+ ByteBuffer headerBuf = readRecord();
+ setAtEOF(isEOFRecord(headerBuf));
+ if (isAtEOF() && headerBuf != null) {
+ // Consume rest
+ tryToConsumeSecondEOFRecord();
+ // TODO: This is present in the TarArchiveInputStream but I don't know if we need this in the random access implementation. All tests are passing ...
+ // consumeRemainderOfLastBlock();
Review comment:
I added the implementation. The test which checks this for the `TarArchiveInputStream` was a bit obscured. I added a bit of documentation so the next one finds it easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-659805215
> In my opinion this adds no value for Commons IO and should not be used outside of the tar implementation.
+1. The `TarArchiveSparseZeroInputStream` will only return 0 if you are reading it. It's simple enough and I think this should be limited to tar in Commons Compress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r551402409
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
##########
@@ -553,6 +554,23 @@ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final
parseTarHeader(headerBuf, encoding, false, lenient);
}
+ /**
+ * Construct an entry from an archive's header bytes for random access tar. File is set to null.
+ * @param headerBuf the header bytes from a tar archive entry.
+ * @param encoding encoding to use for file names.
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link #UNKNOWN}. When set to false such illegal fields cause an exception instead.
+ * @param dataOffset position of the entry data in the random access file.
+ * @since 1.21
+ * @throws IllegalArgumentException if any of the numeric fields have an invalid format.
+ * @throws IOException on error.
+ */
+ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final boolean lenient,
+ final long dataOffset) throws IOException {
+ this(headerBuf, encoding, lenient);
+ this.dataOffset = dataOffset;
+ }
Review comment:
It now throws an IAE if the offset is negative. Detecting if the offset is larger than the complete file is not possible here. I testet setting the offset of an entry to the end of the file. When reading the inputstream there is immediately an IOException thrown.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-757223321
Thank you all for taking the time to review this change!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/35994432)
Coverage increased (+0.08%) to 87.357% when pulling **a0a3ff490dccef9a5a567e87c8e98562c3424d13 on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548928446
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedNIOInputStream.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * NIO backed bounded input stream for reading a predefined amount of data from.
+ * @since 1.21
+ */
+public abstract class BoundedNIOInputStream extends InputStream {
Review comment:
+1
I don't have a better idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-727777619
> In the SparseFilesTest extracting of sparsefile-0.1 in pax_gnu_sparse.tar is skipped because of problems with tar on Ubuntu 16.04. I tested this on Ubuntu 18.04 and everything worked fine. Should we reenable extracting of the file?
Ah. I missed this in my last reviews. It's wired. :)
I tested it on Ubuntu 18.04 and it do works now. That test was failed on Ubuntu 16.04 but unfortunly I don't have a Ubuntu 16.04 now. Agree with @kinow that we can leave it as-is and have it in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r552349616
##########
File path: src/test/java/org/apache/commons/compress/archivers/TarTestCase.java
##########
@@ -335,6 +482,18 @@ public void testCOMPRESS178() throws Exception {
in.close();
}
+ @Test
+ public void testTarFileCOMPRESS178() throws Exception {
+ final File input = getFile("COMPRESS-178.tar");
+ try (final TarFile tarFile = new TarFile(input)) {
+ // Compared to the TarArchiveInputStream all entries are read when instantiating the tar file
+ fail("Expected IOException");
+ } catch (final IOException e) {
Review comment:
I prefer to leave it as is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-726993226
Is anybody able to give me an information how we will progress with this change?
As I currently understand you wanted to hear for more feedback about this changes. Since I will have to do quite some work with the removal of code duplication I'd like to know how long you are willing to wait for feedback.
Also it would be good to get a review of the current state of the changes to guide me into the right direction with the refactorings which need to be done.
I will keep backporting changes from master regarding the TarArchiveInputStream and fix bugs found in the implementation in the meantime since I would like to avoid heading into the wrong direction with any further changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-750851579
Hey @PeterAlfredLee I can invest some time starting from tomorrow. It would be good if you are able to go through the changes once again and comment on the TODOs I left in there.
Regarding the removal of the code duplication in my opinion this would require some major changes to the existing `TarArchiveInputStream`. So we should decide what we want to do about it in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548909890
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
##########
@@ -553,6 +554,23 @@ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final
parseTarHeader(headerBuf, encoding, false, lenient);
}
+ /**
+ * Construct an entry from an archive's header bytes for random access tar. File is set to null.
+ * @param headerBuf The header bytes from a tar archive entry.
+ * @param encoding encoding to use for file names
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link #UNKNOWN}. When set to false such illegal fields cause an exception instead.
+ * @param dataOffset Position of the entry data in the random access file
Review comment:
I formatted all the added Javadoc in `TarArchiveEntry` to start with lower case letter and end with a period.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee merged pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee merged pull request #113:
URL: https://github.com/apache/commons-compress/pull/113
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-757086817
Thanks @theobisproject for your hard work!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/33096947)
Coverage increased (+0.03%) to 87.281% when pulling **94a5620d1987a6951ce3e43e227e5a7c7ec9a906 on theobisproject:random-access-tar** into **08d754cce4bb9a3bc30467e965ab86c64473e032 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-757086817
Thanks @theobisproject for your hard work!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-751972401
The coverage of TarFile looks good enough. Are there any other work left now? @theobisproject
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-654645643
[](https://coveralls.io/builds/35943661)
Coverage increased (+0.1%) to 87.373% when pulling **390031db660304edaa0bd0352613523abfeb7975 on theobisproject:random-access-tar** into **57bcd9b8af628bcb2933c7d80a7ed6e9c5d673e0 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r548909848
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,732 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
+
+ private static final int SMALL_BUFFER_SIZE = 256;
+
+ private final byte[] smallBuf = new byte[SMALL_BUFFER_SIZE];
+
+ private final SeekableByteChannel archive;
+
+ /**
+ * The encoding of the tar file
+ */
+ private final ZipEncoding zipEncoding;
+
+ private final LinkedList<TarArchiveEntry> entries = new LinkedList<>();
+
+ private final int blockSize;
+
+ private final boolean lenient;
+
+ private final int recordSize;
+
+ private final ByteBuffer recordBuffer;
+
+ // the global sparse headers, this is only used in PAX Format 0.X
+ private final List<TarArchiveStructSparse> globalSparseHeaders = new ArrayList<>();
+
+ private boolean hasHitEOF;
+
+ /**
+ * The meta-data about the current entry
+ */
+ private TarArchiveEntry currEntry;
+
+ // the global PAX header
+ private Map<String, String> globalPaxHeaders = new HashMap<>();
+
+ private final Map<String, List<InputStream>> sparseInputStreams = new HashMap<>();
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final String encoding) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final boolean lenient) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive) throws IOException {
+ this(archive.toPath());
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final String encoding) throws IOException {
+ this(archive.toPath(), encoding);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final boolean lenient) throws IOException {
+ this(archive.toPath(), lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final String encoding) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final boolean lenient) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the seekable byte channel to use
+ * @param blockSize the blocks size to use
+ * @param recordSize the record size to use
+ * @param encoding the encoding to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final SeekableByteChannel archive, final int blockSize, final int recordSize, final String encoding, final boolean lenient) throws IOException {
+ this.archive = archive;
+ this.hasHitEOF = false;
+ this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
+ this.recordSize = recordSize;
+ this.recordBuffer = ByteBuffer.allocate(this.recordSize);
+ this.blockSize = blockSize;
+ this.lenient = lenient;
+
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive. This will skip
+ * to the end of the current entry, if there is one, and
+ * place the position of the channel at the header of the
+ * next entry, and read the header and instantiate a new
+ * TarEntry from the header bytes and return that entry.
+ * If there are no more entries in the archive, null will
+ * be returned to indicate that the end of the archive has
+ * been reached.
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private TarArchiveEntry getNextTarEntry() throws IOException {
+ if (isAtEOF()) {
+ return null;
+ }
+
+ if (currEntry != null) {
+ // Skip to the end of the entry
+ archive.position(currEntry.getDataOffset() + currEntry.getSize());
+ throwExceptionIfPositionIsNotInArchive();
+
+ skipRecordPadding();
+ }
+
+ final ByteBuffer headerBuf = getRecord();
+ if (null == headerBuf) {
+ /* hit EOF */
+ currEntry = null;
+ return null;
+ }
+
+ try {
+ currEntry = new TarArchiveEntry(headerBuf.array(), zipEncoding, lenient, archive.position());
+ } catch (final IllegalArgumentException e) {
+ throw new IOException("Error detected parsing the header", e);
+ }
+
+ if (currEntry.isGNULongLinkEntry()) {
+ final byte[] longLinkData = getLongNameData();
+ if (longLinkData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long link entry name not followed by
+ // entry
+ return null;
+ }
+ currEntry.setLinkName(zipEncoding.decode(longLinkData));
+ }
+
+ if (currEntry.isGNULongNameEntry()) {
+ final byte[] longNameData = getLongNameData();
+ if (longNameData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by
+ // entry
+ return null;
+ }
+
+ // COMPRESS-509 : the name of directories should end with '/'
+ final String name = zipEncoding.decode(longNameData);
+ currEntry.setName(name);
+ if (currEntry.isDirectory() && !name.endsWith("/")) {
+ currEntry.setName(name + "/");
+ }
+ }
+
+ if (currEntry.isGlobalPaxHeader()) { // Process Global Pax headers
+ readGlobalPaxHeaders();
+ }
+
+ try {
+ if (currEntry.isPaxHeader()) { // Process Pax headers
+ paxHeaders();
+ } else if (!globalPaxHeaders.isEmpty()) {
+ applyPaxHeadersToCurrentEntry(globalPaxHeaders, globalSparseHeaders);
+ }
+ } catch (NumberFormatException e) {
+ throw new IOException("Error detected parsing the pax header", e);
+ }
+
+ if (currEntry.isOldGNUSparse()) { // Process sparse files
+ readOldGNUSparse();
+ }
+
+ return currEntry;
+ }
+
+ /**
+ * Adds the sparse chunks from the current entry to the sparse chunks,
+ * including any additional sparse entries following the current entry.
+ *
+ * @throws IOException when reading the sparse entry fails
+ */
+ private void readOldGNUSparse() throws IOException {
+ if (currEntry.isExtended()) {
+ TarArchiveSparseEntry entry;
+ do {
+ final ByteBuffer headerBuf = getRecord();
+ if (headerBuf == null) {
+ currEntry = null;
+ break;
+ }
+ entry = new TarArchiveSparseEntry(headerBuf.array());
+ currEntry.getSparseHeaders().addAll(entry.getSparseHeaders());
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ } while (entry.isExtended());
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ /**
+ * Build the input streams consisting of all-zero input streams and non-zero input streams.
+ * When reading from the non-zero input streams, the data is actually read from the original input stream.
+ * The size of each input stream is introduced by the sparse headers.
+ *
+ * NOTE : Some all-zero input streams and non-zero input streams have the size of 0. We DO NOT store the
+ * 0 size input streams because they are meaningless.
+ */
+ private void buildSparseInputStreams() throws IOException {
+ List<InputStream> streams = new ArrayList<>();
+
+ final List<TarArchiveStructSparse> sparseHeaders = currEntry.getSparseHeaders();
+ // sort the sparse headers in case they are written in wrong order
+ if (sparseHeaders != null && sparseHeaders.size() > 1) {
+ final Comparator<TarArchiveStructSparse> sparseHeaderComparator = new Comparator<TarArchiveStructSparse>() {
+ @Override
+ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse q) {
+ Long pOffset = p.getOffset();
+ Long qOffset = q.getOffset();
+ return pOffset.compareTo(qOffset);
+ }
+ };
+ Collections.sort(sparseHeaders, sparseHeaderComparator);
+ }
+
+ if (sparseHeaders != null) {
+ // Stream doesn't need to be closed at all as it doesn't use any resources
+ final InputStream zeroInputStream = new TarArchiveSparseZeroInputStream(); //NOSONAR
+ long offset = 0;
+ long numberOfZeroBytesInSparseEntry = 0;
+ for (TarArchiveStructSparse sparseHeader : sparseHeaders) {
+ if (sparseHeader.getOffset() == 0 && sparseHeader.getNumbytes() == 0) {
+ break;
+ }
+
+ if ((sparseHeader.getOffset() - offset) < 0) {
+ throw new IOException("Corrupted struct sparse detected");
+ }
+
+ // only store the input streams with non-zero size
+ if ((sparseHeader.getOffset() - offset) > 0) {
+ long sizeOfZeroByteStream = sparseHeader.getOffset() - offset;
+ streams.add(new BoundedInputStream(zeroInputStream, sizeOfZeroByteStream));
+ numberOfZeroBytesInSparseEntry += sizeOfZeroByteStream;
+ }
+
+ // only store the input streams with non-zero size
+ if (sparseHeader.getNumbytes() > 0) {
+ long start =
+ currEntry.getDataOffset() + sparseHeader.getOffset() - numberOfZeroBytesInSparseEntry;
+ streams.add(new BoundedSeekableByteChannelInputStream(start, sparseHeader.getNumbytes(), archive));
+ }
+
+ offset = sparseHeader.getOffset() + sparseHeader.getNumbytes();
+ }
+ }
+
+ sparseInputStreams.put(currEntry.getName(), streams);
+ }
+
+ /**
+ * Update the current entry with the read pax headers
+ * @param headers Headers read from the pax header
+ * @param sparseHeaders Sparse headers read from pax header
+ */
+ private void applyPaxHeadersToCurrentEntry(final Map<String, String> headers, final List<TarArchiveStructSparse> sparseHeaders) {
+ currEntry.updateEntryFromPaxHeaders(headers);
+ currEntry.setSparseHeaders(sparseHeaders);
+ }
+
+ /**
+ * For PAX Format 0.0, the sparse headers(GNU.sparse.offset and GNU.sparse.numbytes)
+ * may appear multi times, and they look like:
+ *
+ * GNU.sparse.size=size
+ * GNU.sparse.numblocks=numblocks
+ * repeat numblocks times
+ * GNU.sparse.offset=offset
+ * GNU.sparse.numbytes=numbytes
+ * end repeat
+ *
+ *
+ * For PAX Format 0.1, the sparse headers are stored in a single variable : GNU.sparse.map
+ *
+ * GNU.sparse.map
+ * Map of non-null data chunks. It is a string consisting of comma-separated values "offset,size[,offset-1,size-1...]"
+ *
+ *
+ * For PAX Format 1.X:
+ * The sparse map itself is stored in the file data block, preceding the actual file data.
+ * It consists of a series of decimal numbers delimited by newlines. The map is padded with nulls to the nearest block boundary.
+ * The first number gives the number of entries in the map. Following are map entries, each one consisting of two numbers
+ * giving the offset and size of the data block it describes.
Review comment:
I formatted the Javadoc with html tags. Should be easier to read now when it is rendered
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r456765778
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java
##########
@@ -1106,35 +917,8 @@ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse
}
}
- if (sparseInputStreams.size() > 0) {
+ if (!sparseInputStreams.isEmpty()) {
currentSparseInputStreamIndex = 0;
}
}
-
- /**
- * This is an inputstream that always return 0,
- * this is used when reading the "holes" of a sparse file
- */
- private static class TarArchiveSparseZeroInputStream extends InputStream {
Review comment:
I did think about making the `TarArchiveSparseZeroInputStream` as a separate class instead of a inner class like you do now when I was writing it. I made it a private inner class cause I was thinking that this class could and should only be used here in `TarArchiveInputStream`.
Not sure if making `TarArchiveSparseZeroInputStream` a separate class is a good idea or not.
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedNIOInputStream.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * NIO backed bounded input stream for reading a predefined amount of data from.
+ * @since 1.21
+ */
+public abstract class BoundedNIOInputStream extends InputStream {
Review comment:
The `BoundedInputStream` is renamed as `BoundedNIOInputStream` here. But the matter if it's a NIO input stream is depending on the implemention of abstract method `read` - which is decided by the inherited class.
I don't like the name `BoundedNIOInputStream` as the word 'NIO' is confusing : we need to implement a NIO read to make this stream a NIO input stream.
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedSeekableByteChannelInputStream.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+
+/**
+ * InputStream that delegates requests to the underlying SeekableByteChannel, making sure that only bytes from a certain
+ * range can be read.
+ * @since 1.21
+ */
+public class BoundedSeekableByteChannelInputStream extends BoundedNIOInputStream {
Review comment:
It seems `BoundedSeekableByteChannelInputStream` extends from `BoundedNIOInputStream` is exactly the same as the original inner class `BoundedInputStream` in `ZipFile`. It looks like that `BoundedSeekableByteChannelInputStream` is only used in `ZipFile` now. Is it so?
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,712 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX to archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
Review comment:
Many methods in `TarFile` looks pretty like the ones in `TarArchiveInputStream`(e.g. `buildSparseInputStreams`, `getLongNameData`, `buildSparseInputStreams`). Maybe we can find some way to share these methods?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-656323828
> I am wondering if the introduction of utility streams would not best be done by depending on Apache Commons IO which already contains a bounded stream for example
As I understood commons-compress does purposefully not depend on Commons IO. This is something which I cannot decide to change.
> for BoundedSeekableByteChannelInputStream, please tell users if it is thread-safe in its javadoc.
for BoundedNIOInputStream , please tell users if it is thread-safe in its javadoc.
I will add the information if the general design is accepted from the maintainers.
> for TarArchiveSparseZeroInputStream, maybe this class should be added to commons-io instead.
In my opinion this adds no value for Commons IO and should not be used outside of the tar implementation. I only moved this class to reuse it in both implementations and made it package protected so nobody outside the tar package will use it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r551402409
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
##########
@@ -553,6 +554,23 @@ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final
parseTarHeader(headerBuf, encoding, false, lenient);
}
+ /**
+ * Construct an entry from an archive's header bytes for random access tar. File is set to null.
+ * @param headerBuf the header bytes from a tar archive entry.
+ * @param encoding encoding to use for file names.
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link #UNKNOWN}. When set to false such illegal fields cause an exception instead.
+ * @param dataOffset position of the entry data in the random access file.
+ * @since 1.21
+ * @throws IllegalArgumentException if any of the numeric fields have an invalid format.
+ * @throws IOException on error.
+ */
+ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final boolean lenient,
+ final long dataOffset) throws IOException {
+ this(headerBuf, encoding, lenient);
+ this.dataOffset = dataOffset;
+ }
Review comment:
It now throws an IAE if the offset is negative. Detecting if the offset is larger than the complete file is not possible here. I tested setting the offset of an entry to the end of the file. When reading the inputstream there is immediately an IOException thrown.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-751226385
> > So we should decide what we want to do about it in this PR.
>
> Let's start with the easiest one : there are some conflicts now. Could you resolve them? :)
I resolved the conflicts. As far as I could see there where no additional issues fixed in the new commits for the `TarArchiveInputStream` 😀
> BTW : wish you a great christmas. :-)
Also wish you a merry christmas 🎄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r523908550
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedNIOInputStream.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * NIO backed bounded input stream for reading a predefined amount of data from.
+ * @since 1.21
+ */
+public abstract class BoundedNIOInputStream extends InputStream {
Review comment:
Thanks for explaining your rationale @theobisproject . I don't have a better suggestion, but I agree with @PeterAlfredLee that it is confusing. Perhaps the old name?
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java
##########
@@ -553,6 +554,23 @@ public TarArchiveEntry(final byte[] headerBuf, final ZipEncoding encoding, final
parseTarHeader(headerBuf, encoding, false, lenient);
}
+ /**
+ * Construct an entry from an archive's header bytes for random access tar. File is set to null.
+ * @param headerBuf The header bytes from a tar archive entry.
+ * @param encoding encoding to use for file names
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link #UNKNOWN}. When set to false such illegal fields cause an exception instead.
+ * @param dataOffset Position of the entry data in the random access file
Review comment:
Formatting is inconsistent. Some descriptions start with an upper case letter, others not. Some end with a period, others not. But it's a nit-pick, and can be fixed later :+1:
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,732 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
+
+ private static final int SMALL_BUFFER_SIZE = 256;
+
+ private final byte[] smallBuf = new byte[SMALL_BUFFER_SIZE];
+
+ private final SeekableByteChannel archive;
+
+ /**
+ * The encoding of the tar file
+ */
+ private final ZipEncoding zipEncoding;
+
+ private final LinkedList<TarArchiveEntry> entries = new LinkedList<>();
+
+ private final int blockSize;
+
+ private final boolean lenient;
+
+ private final int recordSize;
+
+ private final ByteBuffer recordBuffer;
+
+ // the global sparse headers, this is only used in PAX Format 0.X
+ private final List<TarArchiveStructSparse> globalSparseHeaders = new ArrayList<>();
+
+ private boolean hasHitEOF;
+
+ /**
+ * The meta-data about the current entry
+ */
+ private TarArchiveEntry currEntry;
+
+ // the global PAX header
+ private Map<String, String> globalPaxHeaders = new HashMap<>();
+
+ private final Map<String, List<InputStream>> sparseInputStreams = new HashMap<>();
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final String encoding) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param content the content to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final byte[] content, final boolean lenient) throws IOException {
+ this(new SeekableInMemoryByteChannel(content), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive) throws IOException {
+ this(archive.toPath());
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final String encoding) throws IOException {
+ this(archive.toPath(), encoding);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the file of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final File archive, final boolean lenient) throws IOException {
+ this(archive.toPath(), lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param encoding the encoding to use
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final String encoding) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding, false);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archivePath the path of the archive to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final Path archivePath, final boolean lenient) throws IOException {
+ this(Files.newByteChannel(archivePath), TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, null, lenient);
+ }
+
+ /**
+ * Constructor for TarFile.
+ *
+ * @param archive the seekable byte channel to use
+ * @param blockSize the blocks size to use
+ * @param recordSize the record size to use
+ * @param encoding the encoding to use
+ * @param lenient when set to true illegal values for group/userid, mode, device numbers and timestamp will be
+ * ignored and the fields set to {@link TarArchiveEntry#UNKNOWN}. When set to false such illegal fields cause an
+ * exception instead.
+ * @throws IOException when reading the tar archive fails
+ */
+ public TarFile(final SeekableByteChannel archive, final int blockSize, final int recordSize, final String encoding, final boolean lenient) throws IOException {
+ this.archive = archive;
+ this.hasHitEOF = false;
+ this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding);
+ this.recordSize = recordSize;
+ this.recordBuffer = ByteBuffer.allocate(this.recordSize);
+ this.blockSize = blockSize;
+ this.lenient = lenient;
+
+ TarArchiveEntry entry;
+ while ((entry = getNextTarEntry()) != null) {
+ entries.add(entry);
+ }
+ }
+
+ /**
+ * Get the next entry in this tar archive. This will skip
+ * to the end of the current entry, if there is one, and
+ * place the position of the channel at the header of the
+ * next entry, and read the header and instantiate a new
+ * TarEntry from the header bytes and return that entry.
+ * If there are no more entries in the archive, null will
+ * be returned to indicate that the end of the archive has
+ * been reached.
+ *
+ * @return The next TarEntry in the archive, or null if there is no next entry.
+ * @throws IOException when reading the next TarEntry fails
+ */
+ private TarArchiveEntry getNextTarEntry() throws IOException {
+ if (isAtEOF()) {
+ return null;
+ }
+
+ if (currEntry != null) {
+ // Skip to the end of the entry
+ archive.position(currEntry.getDataOffset() + currEntry.getSize());
+ throwExceptionIfPositionIsNotInArchive();
+
+ skipRecordPadding();
+ }
+
+ final ByteBuffer headerBuf = getRecord();
+ if (null == headerBuf) {
+ /* hit EOF */
+ currEntry = null;
+ return null;
+ }
+
+ try {
+ currEntry = new TarArchiveEntry(headerBuf.array(), zipEncoding, lenient, archive.position());
+ } catch (final IllegalArgumentException e) {
+ throw new IOException("Error detected parsing the header", e);
+ }
+
+ if (currEntry.isGNULongLinkEntry()) {
+ final byte[] longLinkData = getLongNameData();
+ if (longLinkData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long link entry name not followed by
+ // entry
+ return null;
+ }
+ currEntry.setLinkName(zipEncoding.decode(longLinkData));
+ }
+
+ if (currEntry.isGNULongNameEntry()) {
+ final byte[] longNameData = getLongNameData();
+ if (longNameData == null) {
+ // Bugzilla: 40334
+ // Malformed tar file - long entry name not followed by
+ // entry
+ return null;
+ }
+
+ // COMPRESS-509 : the name of directories should end with '/'
+ final String name = zipEncoding.decode(longNameData);
+ currEntry.setName(name);
+ if (currEntry.isDirectory() && !name.endsWith("/")) {
+ currEntry.setName(name + "/");
+ }
+ }
+
+ if (currEntry.isGlobalPaxHeader()) { // Process Global Pax headers
+ readGlobalPaxHeaders();
+ }
+
+ try {
+ if (currEntry.isPaxHeader()) { // Process Pax headers
+ paxHeaders();
+ } else if (!globalPaxHeaders.isEmpty()) {
+ applyPaxHeadersToCurrentEntry(globalPaxHeaders, globalSparseHeaders);
+ }
+ } catch (NumberFormatException e) {
+ throw new IOException("Error detected parsing the pax header", e);
+ }
+
+ if (currEntry.isOldGNUSparse()) { // Process sparse files
+ readOldGNUSparse();
+ }
+
+ return currEntry;
+ }
+
+ /**
+ * Adds the sparse chunks from the current entry to the sparse chunks,
+ * including any additional sparse entries following the current entry.
+ *
+ * @throws IOException when reading the sparse entry fails
+ */
+ private void readOldGNUSparse() throws IOException {
+ if (currEntry.isExtended()) {
+ TarArchiveSparseEntry entry;
+ do {
+ final ByteBuffer headerBuf = getRecord();
+ if (headerBuf == null) {
+ currEntry = null;
+ break;
+ }
+ entry = new TarArchiveSparseEntry(headerBuf.array());
+ currEntry.getSparseHeaders().addAll(entry.getSparseHeaders());
+ currEntry.setDataOffset(currEntry.getDataOffset() + recordSize);
+ } while (entry.isExtended());
+ }
+
+ // sparse headers are all done reading, we need to build
+ // sparse input streams using these sparse headers
+ buildSparseInputStreams();
+ }
+
+ /**
+ * Build the input streams consisting of all-zero input streams and non-zero input streams.
+ * When reading from the non-zero input streams, the data is actually read from the original input stream.
+ * The size of each input stream is introduced by the sparse headers.
+ *
+ * NOTE : Some all-zero input streams and non-zero input streams have the size of 0. We DO NOT store the
+ * 0 size input streams because they are meaningless.
+ */
+ private void buildSparseInputStreams() throws IOException {
+ List<InputStream> streams = new ArrayList<>();
+
+ final List<TarArchiveStructSparse> sparseHeaders = currEntry.getSparseHeaders();
+ // sort the sparse headers in case they are written in wrong order
+ if (sparseHeaders != null && sparseHeaders.size() > 1) {
+ final Comparator<TarArchiveStructSparse> sparseHeaderComparator = new Comparator<TarArchiveStructSparse>() {
+ @Override
+ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse q) {
+ Long pOffset = p.getOffset();
+ Long qOffset = q.getOffset();
+ return pOffset.compareTo(qOffset);
+ }
+ };
+ Collections.sort(sparseHeaders, sparseHeaderComparator);
+ }
+
+ if (sparseHeaders != null) {
+ // Stream doesn't need to be closed at all as it doesn't use any resources
+ final InputStream zeroInputStream = new TarArchiveSparseZeroInputStream(); //NOSONAR
+ long offset = 0;
+ long numberOfZeroBytesInSparseEntry = 0;
+ for (TarArchiveStructSparse sparseHeader : sparseHeaders) {
+ if (sparseHeader.getOffset() == 0 && sparseHeader.getNumbytes() == 0) {
+ break;
+ }
+
+ if ((sparseHeader.getOffset() - offset) < 0) {
+ throw new IOException("Corrupted struct sparse detected");
+ }
+
+ // only store the input streams with non-zero size
+ if ((sparseHeader.getOffset() - offset) > 0) {
+ long sizeOfZeroByteStream = sparseHeader.getOffset() - offset;
+ streams.add(new BoundedInputStream(zeroInputStream, sizeOfZeroByteStream));
+ numberOfZeroBytesInSparseEntry += sizeOfZeroByteStream;
+ }
+
+ // only store the input streams with non-zero size
+ if (sparseHeader.getNumbytes() > 0) {
+ long start =
+ currEntry.getDataOffset() + sparseHeader.getOffset() - numberOfZeroBytesInSparseEntry;
+ streams.add(new BoundedSeekableByteChannelInputStream(start, sparseHeader.getNumbytes(), archive));
+ }
+
+ offset = sparseHeader.getOffset() + sparseHeader.getNumbytes();
+ }
+ }
+
+ sparseInputStreams.put(currEntry.getName(), streams);
+ }
+
+ /**
+ * Update the current entry with the read pax headers
+ * @param headers Headers read from the pax header
+ * @param sparseHeaders Sparse headers read from pax header
+ */
+ private void applyPaxHeadersToCurrentEntry(final Map<String, String> headers, final List<TarArchiveStructSparse> sparseHeaders) {
+ currEntry.updateEntryFromPaxHeaders(headers);
+ currEntry.setSparseHeaders(sparseHeaders);
+ }
+
+ /**
+ * For PAX Format 0.0, the sparse headers(GNU.sparse.offset and GNU.sparse.numbytes)
+ * may appear multi times, and they look like:
+ *
+ * GNU.sparse.size=size
+ * GNU.sparse.numblocks=numblocks
+ * repeat numblocks times
+ * GNU.sparse.offset=offset
+ * GNU.sparse.numbytes=numbytes
+ * end repeat
+ *
+ *
+ * For PAX Format 0.1, the sparse headers are stored in a single variable : GNU.sparse.map
+ *
+ * GNU.sparse.map
+ * Map of non-null data chunks. It is a string consisting of comma-separated values "offset,size[,offset-1,size-1...]"
+ *
+ *
+ * For PAX Format 1.X:
+ * The sparse map itself is stored in the file data block, preceding the actual file data.
+ * It consists of a series of decimal numbers delimited by newlines. The map is padded with nulls to the nearest block boundary.
+ * The first number gives the number of entries in the map. Following are map entries, each one consisting of two numbers
+ * giving the offset and size of the data block it describes.
Review comment:
I think the comment above could be formatted with HTML to use `<pre>`, `<p>`, etc. But I didn't render the Javadoc to see how it looks like.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-727744498
>In the SparseFilesTest extracting of sparsefile-0.1 in pax_gnu_sparse.tar is skipped because of problems with tar on Ubuntu 16.04. I tested this on Ubuntu 18.04 and everything worked fine. Should we reenable extracting of the file?
+1, if that works with newer versions. Would be good to leave a NOTE/comment somewhere though, in case the test fails again. Even better if we were able to pinpoint what broke it in Ubuntu 16.04 (but can be done later :+1: )
To keep this PR simpler, we can leave as-is, and create a new JIRA to follow-up on this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] XenoAmess commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-655583725
Hi.
suggestions:
for BoundedSeekableByteChannelInputStream, please tell users if it is thread-safe in its javadoc.
for BoundedNIOInputStream , please tell users if it is thread-safe in its javadoc.
for BoundedNIOInputStream , please tell users if it is thread-safe in its javadoc.
for TarArchiveSparseZeroInputStream, maybe this class should be added to commons-io instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-730541671
The benchmark was added to the https://github.com/bodewig/commons-compress-benchmarks repository for easier setup.
```java
package de.samaflost.commons_compress.tar;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.apache.commons.compress.archivers.tar.TarFile;
import org.apache.commons.compress.utils.IOUtils;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.util.NullOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.concurrent.TimeUnit;
@State(Scope.Benchmark)
@Fork(1)
@OutputTimeUnit(TimeUnit.SECONDS)
@Measurement(iterations = 10)
@Warmup(iterations = 10)
public class ReadLargeTarBenchmark {
private InputStream getLargeTarStream() throws IOException {
return Files.newInputStream(getLargeTarPath());
}
private Path getLargeTarPath() {
return Paths.get("path-to-file");
}
private void readEntry(final InputStream inputStream) throws IOException {
IOUtils.copy(inputStream, new NullOutputStream(), 8192);
}
@Benchmark
public void readAllEntries_tarFile() throws IOException {
try (TarFile tarFile = new TarFile(getLargeTarPath())) {
for (TarArchiveEntry entry : tarFile.getEntries()) {
try (InputStream entryInput = tarFile.getInputStream(entry)) {
readEntry(entryInput);
}
}
}
}
@Benchmark
public void readAllEntries_tarStream() throws IOException {
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(getLargeTarStream())) {
TarArchiveEntry entry;
while ((entry = tarIn.getNextTarEntry()) != null) {
readEntry(tarIn);
}
}
}
@Benchmark
public void readFirstEntry_tarFile() throws IOException {
try (TarFile tarFile = new TarFile(getLargeTarPath())) {
try (InputStream entryInput = tarFile.getInputStream(tarFile.getEntries().get(0))) {
readEntry(entryInput);
}
}
}
@Benchmark
public void readFirstEntry_tarStream() throws IOException {
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(getLargeTarStream())) {
tarIn.getNextTarEntry();
readEntry(tarIn);
}
}
@Benchmark
public void readSecondEntry_tarFile() throws IOException {
try (TarFile tarFile = new TarFile(getLargeTarPath())) {
try (InputStream entryInput = tarFile.getInputStream(tarFile.getEntries().get(1))) {
readEntry(entryInput);
}
}
}
@Benchmark
public void readSecondEntry_tarStream() throws IOException {
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(getLargeTarStream())) {
tarIn.getNextTarEntry();
tarIn.getNextTarEntry();
readEntry(tarIn);
}
}
@Benchmark
public void readLastEntry_tarFile() throws IOException {
try (TarFile tarFile = new TarFile(getLargeTarPath())) {
try (InputStream entryInput = tarFile.getInputStream(tarFile.getEntries().get(tarFile.getEntries().size() - 1))) {
readEntry(entryInput);
}
}
}
@Benchmark
public void readLastEntry_tarStream() throws IOException {
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(getLargeTarStream())) {
tarIn.getNextTarEntry();
tarIn.getNextTarEntry();
tarIn.getNextTarEntry();
readEntry(tarIn);
}
}
}
```
This is the result on my machine (Windows 10, Java 11.0.9, File on SSD) with the testfile containing 3 times the Ubuntu 20.04 Desktop image.
```
Benchmark Mode Cnt Score Error Units
ReadLargeTarBenchmark.readAllEntries_tarFile thrpt 10 0,228 ± 0,003 ops/s
ReadLargeTarBenchmark.readAllEntries_tarStream thrpt 10 0,256 ± 0,002 ops/s
ReadLargeTarBenchmark.readFirstEntry_tarFile thrpt 10 0,680 ± 0,010 ops/s
ReadLargeTarBenchmark.readFirstEntry_tarStream thrpt 10 0,766 ± 0,006 ops/s
ReadLargeTarBenchmark.readLastEntry_tarFile thrpt 10 0,689 ± 0,006 ops/s
ReadLargeTarBenchmark.readLastEntry_tarStream thrpt 10 0,127 ± 0,001 ops/s
ReadLargeTarBenchmark.readSecondEntry_tarFile thrpt 10 0,690 ± 0,012 ops/s
ReadLargeTarBenchmark.readSecondEntry_tarStream thrpt 10 0,218 ± 0,003 ops/s
```
I think this demonstrates the strength and weakness of the implementation.
**Strength**
- Performance of random access is constant
- Extracting all entries is near to the performance of the `TarArchiveStream`
**Weakness**
- Only extracting the first entry is slower because of the extra work which is done for the random access
The performance gap should be smaller when the archive is smaller. Feel free to experiment with different tar contents and number of files and report back your result.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-752681318
I removed the code. Now everything I wanted to address is done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a change in pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
theobisproject commented on a change in pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#discussion_r457052857
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java
##########
@@ -1106,35 +917,8 @@ public int compare(final TarArchiveStructSparse p, final TarArchiveStructSparse
}
}
- if (sparseInputStreams.size() > 0) {
+ if (!sparseInputStreams.isEmpty()) {
currentSparseInputStreamIndex = 0;
}
}
-
- /**
- * This is an inputstream that always return 0,
- * this is used when reading the "holes" of a sparse file
- */
- private static class TarArchiveSparseZeroInputStream extends InputStream {
Review comment:
I moved this class out of the `TarArchiveInputStream` because I also needed it in the `TarFile`. Otherwise I would had to introduce more duplicated code. I made the class package private to prevent accidental usage.
In my opinion before the changes done here it was reasonable to have this as inner class as this was only used here.
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedNIOInputStream.java
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * NIO backed bounded input stream for reading a predefined amount of data from.
+ * @since 1.21
+ */
+public abstract class BoundedNIOInputStream extends InputStream {
Review comment:
I agree that the name of the class it not perfect. Just wan't to share the reason why I took the name.
The newly defined read method reads the content into an `java.nio.ByteBuffer` and the `read` methods of the `InputStream` are calling it. So the name comes more from the fact that an inherited class only needs to implement the new `read` method to the `ByteBuffer` and not from the actual underlying implementation. My experience shows that reading into a ByteBuffer from a non NIO source is not so much fun and therefore I put the `NIO` in the classname.
If someone has a better suggestion for a classname it would be welcome.
##########
File path: src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java
##########
@@ -0,0 +1,712 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.tar;
+
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.compress.archivers.zip.ZipEncoding;
+import org.apache.commons.compress.archivers.zip.ZipEncodingHelper;
+import org.apache.commons.compress.utils.ArchiveUtils;
+import org.apache.commons.compress.utils.BoundedInputStream;
+import org.apache.commons.compress.utils.BoundedNIOInputStream;
+import org.apache.commons.compress.utils.BoundedSeekableByteChannelInputStream;
+import org.apache.commons.compress.utils.SeekableInMemoryByteChannel;
+
+/**
+ * The TarFile provides random access to UNIX to archives.
+ * @since 1.21
+ */
+public class TarFile implements Closeable {
Review comment:
The duplication is really on purpose from my side in the current state of the PR. I didn't wanted to limit myself to early from making the needed changes to get this work. Also I wanted to limit changes to existing code to avoid breaking it. Sharing some of the code can be quite a challenge because the data source is so different.
`buildSparseInputStream`: The difference here is that the sparse streams are saved differently in the TarFile than in the Stream. In the TarFile the streams need to be saved on a per entry basis to allow random access. I think this could also be done in the stream but for the first version I didn't wanted to change too much existing code.
`getLongNameData`: The only difference is the source from where the data is read (InputStream vs. SeekableByteChannel). A problem could be the call to `getNext(Tar)Entry` which has a lot of sideeffects.
##########
File path: src/main/java/org/apache/commons/compress/utils/BoundedSeekableByteChannelInputStream.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.utils;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+
+/**
+ * InputStream that delegates requests to the underlying SeekableByteChannel, making sure that only bytes from a certain
+ * range can be read.
+ * @since 1.21
+ */
+public class BoundedSeekableByteChannelInputStream extends BoundedNIOInputStream {
Review comment:
You are correct about the origin of the code. I moved it out of the `ZipFile` class because it is needed for reading sparse tar conent (see `TarFile` line 377)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #113: COMPRESS-540: Implement TarFile to allow random access to tar files
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #113:
URL: https://github.com/apache/commons-compress/pull/113#issuecomment-699962152
IMO, it's OK to have a random access for Tar - even through it would be slower as we read the `tar archive` to get the positions of entries. But I think we should hear more voice from others about this idea, cause this is a big change and `tar archives` are not designed to have a random access.
@theobisproject And as you said, there are still some work(e.g. the refactor of the duplication) to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org