Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More tests #191

Merged
merged 12 commits into from
Sep 19, 2023
Merged

More tests #191

merged 12 commits into from
Sep 19, 2023

Conversation

alexlarsson
Copy link
Collaborator

This adds some fusa/tools tweaks and then some initial work on randomized image testing.
For now it only uses fuse so we can run this during unit tests, but I want to follow up with similar integration tests that do an actual kernel mount.

@alexlarsson alexlarsson force-pushed the more-tests branch 9 times, most recently from 550a1c7 to 502472f Compare September 19, 2023 10:19
@alexlarsson
Copy link
Collaborator Author

Hmm, seems like fsck.erofs is not big endian safe, at least not the version in ubuntu.

i_mtime_nsec is 32bit, so use lcfs_u32_to_file, not lcfs_u64_to_file.
The later was breaking on big endian arches.

Signed-off-by: Alexander Larsson <[email protected]>
This allows you to build an image with user xattrs, but not get things
like e.g. selinux contexts.

Signed-off-by: Alexander Larsson <[email protected]>
Like --skip-xattrs, but keeps user xattrs.

Signed-off-by: Alexander Larsson <[email protected]>
We don't want to rewrite trusted.overlay to user.overlay anymore, as
that was a leftover from when using the fuse fs combined with overlayfs,
instead we need to support xattr escapes.

Also, until fuse supports non-user xattrs, don't return them from the
fuse APIs, because listing the xattr will work, but the getattr will
then be filtered out and get ENODATA.

Signed-off-by: Alexander Larsson <[email protected]>
This gets rid of the 00-ff files we generate

Signed-off-by: Alexander Larsson <[email protected]>
Signed-off-by: Alexander Larsson <[email protected]>
Pass in $VALGRIND_PREFIX separately rather than via $BINDIR. This means
we don't have to valgrind everything in $BINDIR, which will be useful
later.

Signed-off-by: Alexander Larsson <[email protected]>
These options tweak the output of dumpdir, similar to what we expect
some composefs tools to do.

In particular:

--userxattrs:
  Only dump user.* xattrs.

  This is what composefs-fuse will do.

--noescaped:

  Don't dump escaped xattrs.

  This is what a real composefs mount will do if kernel overlayfs doesn't
  support xattr escapes.

--whiteouts:

  Convert regular whiteouts to xattr-based whiteouts.

  This is what mkcomposefs does to whiteouts.

Signed-off-by: Alexander Larsson <[email protected]>
Signed-off-by: Alexander Larsson <[email protected]>
Generates 10 random images and runs some test on them.

The tests run are:
 * Dump (parse and re-write it) image and esure we get identical results
 * Run fsck.erofs on image
 * Mount image using fuse and ensure dumpdir is the same
   (sans non-user-xattr and whiteout)
 * Run mkcomposefs on the fuse mount and ensure the result is similar.
   (It produces same fuse mount dump, but due to non-user xattrs and
    whiteouts its not producing an identical composefs image)

Signed-off-by: Alexander Larsson <[email protected]>
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks sane, but I didn't deep dive into the tests or the xattr changes.

@@ -881,7 +881,7 @@ static int write_erofs_inode_data(struct lcfs_ctx_s *ctx, struct lcfs_node_s *no
i.i_uid = lcfs_u32_to_file(node->inode.st_uid);
i.i_gid = lcfs_u32_to_file(node->inode.st_gid);
i.i_mtime = lcfs_u64_to_file(node->inode.st_mtim_sec);
i.i_mtime_nsec = lcfs_u64_to_file(node->inode.st_mtim_nsec);
i.i_mtime_nsec = lcfs_u32_to_file(node->inode.st_mtim_nsec);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, good old C automatic integer promotion rules, here to blow off our feet.

So I tested this, and it finds the problem:

diff --git a/libcomposefs/lcfs-internal.h b/libcomposefs/lcfs-internal.h
index f9cab47..749dbe6 100644
--- a/libcomposefs/lcfs-internal.h
+++ b/libcomposefs/lcfs-internal.h
@@ -70,10 +70,10 @@ static inline uint32_t lcfs_u32_to_file(uint32_t val)
 	return htole32(val);
 }
 
-static inline uint64_t lcfs_u64_to_file(uint64_t val)
-{
-	return htole64(val);
-}
+#define lcfs_u64_to_file(v) ({ \
+	_Static_assert(sizeof(v) == sizeof(uint64_t)); \
+	htole64(v); \
+})
 
 static inline uint16_t lcfs_u16_from_file(uint16_t val)
 {

Opinions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may hit problematic cases where we e.g. pass an int to u64_to_file(), but I'm fine with having those explicitly add a cast as needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme try it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,195 @@
#!/usr/bin/python3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repository is now up to 3 distinct languages (C, shell script, and Python). Personally, I would vote that eventually of using Python and shell script for tests we use Rust. I've had decent success with this, see e.g. https://github.com/ostreedev/ostree/blob/878d601665fd15781adf774d99dd86e9a172cb92/tests/inst/src/destructive.rs#L477 shows how Rust's macros can let one execute shell commands (xshell crate) with near-equal ergonomics to bash, but more safety than Python.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love rust for production code, but I'm not sure how well it would fit with code that is a bit more throwaway and like tests. Seems like it may raise the bar for people to add new testcases. I'd be willing to be convinced though.

@alexlarsson alexlarsson merged commit 420cd38 into containers:main Sep 19, 2023
8 checks passed
@hsiangkao
Copy link
Contributor

Hmm, seems like fsck.erofs is not big endian safe, at least not the version in ubuntu.

I don't quite have an environment to test big-endian... I know I could setup a QEMU environment, but honestly currently I don't have enough time on this now..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants