Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for long xattr prefixes to the EROFS filesystem implementation, which was previously unimplemented and would return an error. The changes include reading and caching long prefixes from the filesystem metadata and updating the xattr parsing logic to handle them.
Key changes:
- Replaces TODO error returns with actual long prefix support in xattr parsing
- Adds caching mechanism for long prefixes with lazy loading from filesystem metadata
- Includes comprehensive test coverage for files with long xattr prefixes
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| xattr.go | Replaces unimplemented long prefix errors with calls to retrieve cached long prefixes |
| erofs.go | Adds long prefix loading and caching infrastructure with detailed parsing logic |
| erofs_test.go | Adds test cases for files with long xattr prefixes to verify the new functionality |
| README.md | Updates documentation to reflect completed long xattr prefix support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
erofs.go
Outdated
| return | ||
| } | ||
|
|
||
| // Long prefixes are stored in the packed inode, after the inode header |
There was a problem hiding this comment.
Hi Derek @dmcgowan, long xattr prefixes (Linux 6.4~6.16)were defined as:
if INCOMPAT_FRAGMENTS is set, the long prefix table is located in the packed inode (XattrPrefixStart * 4);
if INCOMPAT_FRAGMENTS is not set, the long prefix table is directly in the ondisk offset (XattrPrefixStart * 4);
However this way is flew because:
- The packed inode is used to compress unaligned tail data together for example, but some users may not need to compress the long xattr prefixes (although some part of the packed inode can be specified as uncompressed);
- Since metadata compression is introduced (INCOMPAT_METABOX, Linux 6.17+, including this year LTS), a dedicated metadata (metabox) inode is introduced to contain compressed metadata, so long xattr prefixes would be better to be located in the metabox too.
So long xattr prefixes placement is fixed as: https://git.kernel.org/xiang/erofs-utils/c/0cf1039782b2
I admit it was messy initially due to lack of more deeply thoughts when this feature was firstly landed, but it's now fixed.
the implementation can be refered: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/tree/lib/xattr.c?id=0cf1039782b2#n1680
and
https://git.kernel.org/torvalds/c/1fcf686def19
erofs.go
Outdated
| // The packed inode is typically a compact inode (32 bytes) | ||
| // Address = MetaBlkAddr + (PackedNid * 32) + inode_size + (XattrPrefixStart * 4) | ||
| // Note: XattrPrefixStart is in units of 4 bytes from the end of the packed inode | ||
| baseAddr := img.blkOffset() + int64(img.sb.PackedNid)*32 + 32 + int64(img.sb.XattrPrefixStart)*4 |
There was a problem hiding this comment.
If long xattr prefixes are located in packed inode or metabox inode, I suggest just using regular inode read logic to read XattrPrefixStart*4 directly, instead of just read data after the inode metadata (because the packed inode can be compressed, or non-inline, etc.)
dmcgowan
force-pushed
the
long-prefix-support
branch
2 times, most recently
from
4d7c8d7 to
2b43857
Compare
|
Updated |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Let's just merge as-is, I need to refactor some part e.g. directory data read should go through regular file read (rathar than directly read on-disk original data), since directories are special files and they support compression too |
|
Also, I'm not sure if we'd like to keep test cases in erofs binaries, since previous xz backdoor just leverages random binary data in the source code, it would be helpful to get erofs from scratch. Btw, this commit needs rebasing so that it can be merged. |
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
long-prefix-support
branch
from
2b43857 to
91f6c2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if int(index) >= len(img.longPrefixes) { | ||
| return "", fmt.Errorf("long xattr prefix index %d out of range (max %d)", index, len(img.longPrefixes)-1) |
There was a problem hiding this comment.
When img.longPrefixes is empty (e.g., XattrPrefixCount==0 but an xattr entry still has the long-prefix bit set), this error message reports max -1, which is confusing.
Consider special-casing len(img.longPrefixes)==0 to return a clearer error like “no long xattr prefixes present/loaded”, and otherwise report the valid index range.
| if int(index) >= len(img.longPrefixes) { | |
| return "", fmt.Errorf("long xattr prefix index %d out of range (max %d)", index, len(img.longPrefixes)-1) | |
| if len(img.longPrefixes) == 0 { | |
| return "", fmt.Errorf("no long xattr prefixes present/loaded (requested index %d)", index) | |
| } | |
| if int(index) >= len(img.longPrefixes) { | |
| return "", fmt.Errorf("long xattr prefix index %d out of range (valid 0..%d)", index, len(img.longPrefixes)-1) |
|
@hsiangkao sorry, I didn't push the version with the update before, it is updated now using the new read logic |
| // - The infix string of length prefixLen - 1 | ||
| // - Padding to align the entire entry (2 + prefixLen) to a 4-byte boundary. | ||
| var lenBuf [2]byte | ||
| if _, err := io.ReadFull(r, lenBuf[:]); err != nil { |
There was a problem hiding this comment.
2-byte(len) + len-bytes(buffer) will be used to parse variable-sized structures like
long prefix xattr table and compression configuration table
I hope there could be a generic helper to parse this structure like:
https://github.com/torvalds/linux/blob/master/fs/erofs/super.c#L90
2 participants
Updates testdata to include long prefixes and related tests. In first commit tests are added and fails. The second commit adds support for the prefixes, reading the entire set and caching the prefixes.