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.

Sign up for GitHub

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

Jump to bottom

cryptsetup: revert last update #46151

Closed
wants to merge 2 commits into from
Closed

cryptsetup: revert last update #46151

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed77b69
Revert "cryptsetup: 2.0.3 -> 2.0.4"
oxij Sep 6, 2018
1f5d05e
cryptsetup: enable tests
oxij Sep 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkgs/os-specific/linux/cryptsetup/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ stdenv.mkDerivation rec {
buildInputs = [ lvm2 json_c openssl libuuid popt ]
++ stdenv.lib.optional enablePython python2;

doCheck = true;
Copy link
Member

Choose a reason for hiding this comment

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

What was the failure message?

Copy link
Member

@Mic92 Mic92 Sep 6, 2018 •
edited

Choose a reason for hiding this comment

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

Ideally we won't stuck on older versions forever or else we cannot get security updates.

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails most "compat" tests with some strange "file not found" errors. It might be just a bug in the tests, but I would feel uneasy running cryptsetup that could break something that is "compat".

Copy link
Member

Choose a reason for hiding this comment

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

Is this with or without sandbox? The latest cryptsetup also checks block devices for other filesystems and prevent corruption that way. That's means I would also feel uneasy to not having this safety feature.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot reproduce the failure. Tests works for me: https://gist.github.com/Mic92/a603aea57ecd079b76903a54d3e1dd90#file-cryptsetup-log-L406

Copy link
Member

@Mic92 Mic92 Sep 7, 2018 •
edited

Choose a reason for hiding this comment

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

ok, File not found could be the script itself, please try to use patchShebangs instead:

https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/compat-test#L1

Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be 2 issues here:

  1. The file not found error above is sandbox-related (although patchShebangs is already used). Still trying to find the reason.

  2. With sandboxing disabled, the compat test runs and succeeds if the build directory is on an ext4 fs, but 2 test cases fail if the build directory is on a btrfs fs (which is my default here). This is weird. We cannot have test success depend on the what fs the build dir is on, so we will need to disable the failing cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, actually it's only one issue: the test uses an internal tool unit-utils-io to read/write blocks in the file blocwise_localfile. This file exists but unit-utils-io opens it with O_DIRECT https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/unit-utils-io.c#L92, which is forbidden on a tmpfs and on btrfs (which produces the file not found errors - tmpfs in sandbox, btrfs without sandbox) but allowed on ext4. This is just stupid.

@oxij @Mic92 what should we do? We could

  • revert to 2.0.3
  • just disable this filesystem-dependent test
  • create a nixos test to run complete the tests in a VM where we can control the file system type.

Copy link
Member

Choose a reason for hiding this comment

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

Either

  • disable this filesystem-dependent test
  • create a nixos test to run complete the tests in a VM where we can control the file system type.

O_DIRECT is not implemented on zfs. It is a braindead interface anyway.
Reverting just because a test does not work in our build environment is pointless as the test works perfectly fine without it.
Actually we already have integration tests for cryptsetup. And don't think we need to duplicate upstream testing beyond a certain level in our own systems. Most of the critical stuff in cryptsetup is implemented in the kernel anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm implementing a solution to patch out O_DIRECT and disable the remaining 4 test cases. PR will follow shortly.


meta = {
homepage = https://gitlab.com/cryptsetup/cryptsetup/;
description = "LUKS for dm-crypt";
Expand Down