std.tar: error on insufficient buffers provided to iterator #19155
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewrk
requested changes
Mar 8, 2024
lib/std/tar.zig
Outdated
Comment on lines
897
to
900
| // With test enabled this is hanging under windows: | ||
| // zig build test docs --zig-lib-dir .\lib\ -Dstatic-llvm -Dskip-non-native -Denable-symlinks-windows | ||
| const builtin = @import("builtin"); | ||
| if (builtin.os.tag == .windows) return error.SkipZigTest; |
There was a problem hiding this comment.
When disabling a test, please first file an issue, then link to the issue when disabling it.
There was a problem hiding this comment.
I traced problem a little bit more and file the issue.
I fixed this test. The problem was that created files were not closed.
andrewrk
requested changes
Mar 9, 2024
lib/std/tar.zig
Outdated
Comment on lines
253
to
259
| /// var file_name_buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; | ||
| /// var link_name_buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; | ||
| /// | ||
| /// var iter = std.tar.iterator(archive.reader(), .{ | ||
| /// .file_name_buffer = &file_name_buffer, | ||
| /// .link_name_buffer = &link_name_buffer, | ||
| /// }); |
There was a problem hiding this comment.
Don't put code in doc comments. Instead, use a doctest like this:
test iterator {
...
}Then it will show up as example usage like this:
This will require the test to be valid code that passes the tests, and therefore cannot bitrot.
ianic
force-pushed
the
tar_max_file_size
branch
from
35c7ef8 to
44da9be
Compare
Should do that before I broke package manager!
Tar header stores name in max 256 bytes and link name in max 100 bytes. Those are minimums for provided buffers. Error is raised during iterator init if buffers are not long enough. Pax and gnu extensions can store longer names. If such extension is reached during unpack and don't fit into provided buffer error is returned.
Don't assert min buffer size on iterator init. That was changing public interface. This way we don't break that interface.
for the then end users: 1. Don't require user to call file.skip() on file returned from iterator.next if file is not read. Iterator will now handle this. Previously that returned header parsing error, without knowing some tar internals it is hard to understand what is required from user. 2. Use iterator.File.kind enum which is similar to fs.File.Kind, something familiar. Internal Header.Kind has many types which are not exposed but the user needs to have else in kind switch to cover those cases. 3. Add reader interface to the iterator.File.
Fixing error from ci: std/tar.zig:423:54: error: expected type 'usize', found 'u64' std/tar.zig:423:54: note: unsigned 32-bit int cannot represent all possible unsigned 64-bit values
Fixing ci error: error: 'tar.test.test.pipeToFileSystem' failed: slices differ. first difference occurs at index 2 (0x2) ============ expected this output: ============= len: 9 (0x9) 2E 2E 2F 61 2F 66 69 6C 65 ../a/file ============= instead found this: ============== len: 9 (0x9) 2E 2E 5C 61 5C 66 69 6C 65 ..\a\file After ziglang#19136 dir.symlink changes path separtors to \ on windows.
When this test is enabled something like: `zig build test docs --zig-lib-dir .\lib\ -Dstatic-llvm -Dskip-non-native -Denable-symlinks-windows` never finishes. Those are failed runs from ci: https://github.com/ziglang/zig/actions/runs/8137710393 https://github.com/ziglang/zig/actions/runs/8129619923 https://github.com/ziglang/zig/actions/runs/8125845128 Isolating that test and running it is not a problem. Running something like `zig test .\lib\std\std.zig --zig-lib-dir .\lib\` is fine.
Problem was manifested only on windows with target `-target aarch64-windows-gnu`. I was creating new files but not closing any of them. Tmp dir cleanup hangs looping in deleteTree forever.
Make std.tar look better in docs. Remove from public interface what is not necessary. Add comment to the public methods. Add doctest as usage examples for iterator and pipeToFileSystem.
It is no longer need to call skip if file content is not consumed. It is handled internally now. File types are now same as in os.File.
ianic
force-pushed
the
tar_max_file_size
branch
from
ede03b5 to
c974e19
Compare
|
Thanks for the docs fixes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is follow up of #19128 where we changed iterator interface to accept buffers from caller. Internally we were assuming that buffers are big enough. Now when they are provided from outside we are checking that and raise error if there is not enough space.
I also make few changes to make iterator more ergonomic to the end users:
Don't require user to call file.skip() on file returned from iterator.next if it is not read. Iterator will now handle this.
Previously that returned header parsing error, without knowing some tar internals it is hard to understand what is required from user.
Use iterator.File.kind enum which is similar to fs.File.Kind. Internal Header.Kind has many types which are not exposed but the user needed to have else in kind switch to cover those cases. Now there are only three kind of iterator.File: .directory, .file, .sym_link.
Add reader interface to the iterator.File.