reorganize code and add an unit test #354
Conversation
Reorganize the code to make the structure clearer. Specifically: - erofs_common.*: contains some common content used internally by EROFS, such as I/O controllers and constants; - liberofs.*: contains the content for creating EROFS filesystem images; - erofs_fs.*: contains the content for building the EROFS filesystem from EROFS filesystem images and performing operations on the filesystem. Additional changes: - rename `min` to `erofs_min` to avoid conflicts with `std::min`; - declare erofs_check_fs() and erofs_create_fs() as extern in comm_func.cpp, as including erofs_fs.h at this point leads to conflicts. Subsequent patches will clean up erofs_fs.h to allow it to be included by third parties (hide content related to liberofs to avoid conflicts). No other changes were made. Signed-off-by: Hongzhen Luo <[email protected]>
src/overlaybd/tar/erofs/erofs_fs.cpp
Outdated
| struct erofs_fs_private { | ||
| struct erofs_sb_info sbi; | ||
| struct liberofs_file target_file; | ||
| unsigned long magic = LIBEROFS_MAGIC; |
There was a problem hiding this comment.
what's the exact use of this field?
There was a problem hiding this comment.
what's the exact use of this fields?
The ErofsFileSystem needs to be associated with the erofs_superblock and the I/O controller, so this information has been packaged here. The same goes for ErofsFile. The reason for this approach is to avoid including items like erofs_superblock in erofs_fs.h.
There was a problem hiding this comment.
I mean what's the uses of magic?
There was a problem hiding this comment.
Just a double check for pointer conversion using reinterpret_cast.
There was a problem hiding this comment.
I don't think it's useful
There was a problem hiding this comment.
Done. This field is removed.
src/overlaybd/tar/erofs/erofs_fs.h
Outdated
|
|
||
| class ErofsFileSystem: public photon::fs::IFileSystem { | ||
| public: | ||
| struct erofs_sb_info sbi; | ||
| struct liberofs_file target_file; | ||
| void *fs_private; |
There was a problem hiding this comment.
I think you could just
..
private:
struct ErofsFileSystemInt;
ErofsFileSystemInt *private;
src/overlaybd/tar/erofs/erofs_fs.h
Outdated
| @@ -61,7 +58,7 @@ class ErofsFileSystem: public photon::fs::IFileSystem { | |||
| class ErofsFile: public photon::fs::VirtualReadOnlyFile { | |||
| public: | |||
| ErofsFileSystem *fs; | |||
| struct erofs_inode inode; | |||
| void *file_private; | |||
There was a problem hiding this comment.
I think you could just
...
private:
struct ErofsFileInt;
ErofsFileInt *private;There was a problem hiding this comment.
OK, I will give it a try.
Hide the parts related to liberofs in erofs_fs.h so that erofs_fs.h can be included. Signed-off-by: Hongzhen Luo <[email protected]>
Add an unit test for the internal ErofsCache. Signed-off-by: Hongzhen Luo <[email protected]>
|
LGTM |
What this PR does / why we need it:
Reorganize the code to make the structure clearer. And also add an unit test for the internal ErofsCache.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Please check the following list: