crypto: Allow using activate flags when opening a device #1150
Conversation
Summary of ChangesHello @vojtechtrefny, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change adds a new bitfield type Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable enhancement by adding *_open_flags functions, allowing activation flags like READONLY and ALLOW_DISCARDS when opening encrypted devices. The implementation thoughtfully maintains backward compatibility. While the changes are mostly well-tested, I've identified a few issues, including a critical syntax error, a bug in the Python overrides, some documentation mistakes, and opportunities to improve test coverage for the new READONLY flag.
| BD_CRYPTO_LUKS_ACTIVATE_HIGH_PRIORITY = 1 << 6, | ||
| } BDCryptoLUKSPersistentFlags; | ||
|
|
||
| typedef enum { |
There was a problem hiding this comment.
cryptsetup has just one set of "flags", some of these can be set persistently for LUKS, some can be used for crypt_activate and some for some other functionality. Looking at the BDCryptoLUKSPersistentFlags flags, it might be better to make the flag names more specific for the "persistent flag" use case, but it's too late now and so the BDCryptoOpenFlags flag names might look a bit weird. But if you have a better idea for naming these, I am definitely open to suggestions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/python/gi/overrides/BlockDev.py (1)
347-361: Consider adding*_open_flagswrappers for BitLocker and FileVault2.The existing
crypto_bitlk_openandcrypto_fvault2_openwrappers handle passphrase-to-bytes encoding. For consistency, similar wrappers forcrypto_bitlk_open_flagsandcrypto_fvault2_open_flagsshould be added if they will be called from Python with string passphrases.Example for BitLocker:
_crypto_bitlk_open_flags = BlockDev.crypto_bitlk_open_flags @override(BlockDev.crypto_bitlk_open_flags) def crypto_bitlk_open_flags(device, name, passphrase, flags=0): if isinstance(passphrase, str): passphrase = passphrase.encode("utf-8") return _crypto_bitlk_open_flags(device, name, passphrase, flags) __all__.append("crypto_bitlk_open_flags")tests/crypto_test.py (1)
1519-1530: Consider adding READONLY test for VeraCrypt.The VeraCrypt test only verifies
ALLOW_DISCARDS. For consistency with BitLocker and FileVault2 tests, consider testing theREADONLYflag as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/libblockdev-sections.txt(2 hunks)src/lib/plugin_apis/crypto.api(5 hunks)src/plugins/crypto.c(20 hunks)src/plugins/crypto.h(3 hunks)src/python/gi/overrides/BlockDev.py(1 hunks)tests/crypto_test.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/crypto_test.py (2)
src/python/gi/overrides/BlockDev.py (2)
CryptoKeyslotContext(284-300)crypto_tc_open_flags(341-344)tests/utils.py (1)
run_command(561-566)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (46)
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: compilation (clang-15)
- GitHub Check: compilation (gcc-13)
- GitHub Check: compilation (clang-16)
- GitHub Check: compilation (clang-18)
- GitHub Check: compilation (gcc-10)
- GitHub Check: compilation (gcc-11)
- GitHub Check: compilation (clang-14)
- GitHub Check: compilation (clang-17)
- GitHub Check: compilation (gcc-14)
- GitHub Check: compilation (gcc-12)
- GitHub Check: udisks-build
- GitHub Check: Analyze (cpp)
- GitHub Check: symbols
- GitHub Check: Analyze (python)
- GitHub Check: blivet-tests
- GitHub Check: csmock
🔇 Additional comments (15)
src/plugins/crypto.h (2)
175-178: LGTM: Flag enum is well-defined.The
BDCryptoOpenFlagsenum uses proper bit-shift values for flags, allowing them to be combined with bitwise OR. This aligns with the existingBDCryptoIntegrityOpenFlagspattern in the codebase.
298-298: LGTM: New flag-based open function declarations are consistent.The new
*_open_flagsfunction declarations correctly replace thegboolean read_onlyparameter withBDCryptoOpenFlags flags, maintaining backward compatibility by keeping the original functions unchanged.Also applies to: 328-328, 332-332, 336-336
src/python/gi/overrides/BlockDev.py (1)
341-344: Consider: Wrapper uses raw passphrase instead of context object.The
crypto_tc_open_flagswrapper accepts a rawpassphraseparameter and encodes it internally. However, looking at the C API (bd_crypto_tc_open_flags), it expects aBDCryptoKeyslotContext. This wrapper may need to construct a context object from the passphrase, similar toCryptoKeyslotContext(passphrase=passphrase).Verify that passing a raw encoded passphrase works correctly with the underlying GI binding, or consider creating a context object internally.
tests/crypto_test.py (4)
120-127: LGTM: Helper method for read-only state verification.The
_is_rohelper correctly resolves the dm-mapper device path and reads theroattribute from sysfs to verify read-only state.
714-738: LGTM: Test coverage for LUKS open with flags.The tests verify that
ALLOW_DISCARDSflag correctly enables discard support on the mapped device viadmsetup tableinspection.
1631-1644: LGTM: BitLocker open with flags test.The test correctly verifies both
ALLOW_DISCARDS(via dmsetup) andREADONLY(via_is_ro) flags work together.
1745-1758: LGTM: FileVault2 open with flags test.The test correctly verifies both
ALLOW_DISCARDSandREADONLYflags, consistent with the BitLocker test.src/lib/plugin_apis/crypto.api (2)
364-367: LGTM: Flag enum definition in API.The
BDCryptoOpenFlagsenum is correctly defined with the same values as in the header file.
970-970: LGTM: New flag-based open function declarations.The API function declarations for
bd_crypto_luks_open_flags,bd_crypto_tc_open_flags,bd_crypto_bitlk_open_flags, andbd_crypto_fvault2_open_flagsare correctly defined with theBDCryptoOpenFlagsparameter.Also applies to: 1352-1352, 1410-1410, 1453-1453
src/plugins/crypto.c (6)
1264-1374: LGTM! Flag-based activation logic is correct.The new
bd_crypto_luks_open_flagsfunction correctly mapsBDCryptoOpenFlagsto cryptsetup's activation flags and passes them to all activation code paths (passphrase, keyfile, keyring).
3194-3303: LGTM! TrueCrypt/VeraCrypt flag support is correctly implemented.The flag conversion logic is consistent with the LUKS implementation and correctly passes activation flags to cryptsetup.
3305-3327: LGTM! Wrapper maintains backward compatibility.The wrapper correctly delegates to
bd_crypto_tc_open_flagswith proper flag conversion.
3589-3687: LGTM! BitLocker flag support is correctly implemented.The implementation is consistent with other device types and correctly handles both passphrase and keyfile activation paths.
3689-3706: LGTM! Wrapper correctly delegates to the flags variant.
3721-3826: LGTM! FileVault2 flag support is correctly implemented.The implementation properly handles conditional compilation and follows the same pattern as other device types.
vojtechtrefny
force-pushed
the
master_luks-open-flags
branch
from
aa0ca48 to
c3415f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/crypto.c (1)
3721-3851: Missing closing brace inbd_crypto_fvault2_openwhenLIBCRYPTSETUP_26is not defined.In the
#ifndef LIBCRYPTSETUP_26branch ofbd_crypto_fvault2_open, the function body is never closed; the only}is inside the#elsebranch. When compiling withoutLIBCRYPTSETUP_26, this will produce a syntax error.You can fix this by adding a closing brace before the
#else, so each branch defines a complete function:#ifndef LIBCRYPTSETUP_26 gboolean bd_crypto_fvault2_open (const gchar *device G_GNUC_UNUSED, const gchar *name G_GNUC_UNUSED, BDCryptoKeyslotContext *context G_GNUC_UNUSED, - gboolean read_only G_GNUC_UNUSED, GError **error) { - /* this will return FALSE and set error, because FVAULT2 technology is not available */ - return bd_crypto_is_tech_avail (BD_CRYPTO_TECH_FVAULT2, BD_CRYPTO_TECH_MODE_OPEN_CLOSE, error); -#else + gboolean read_only G_GNUC_UNUSED, GError **error) { + /* this will return FALSE and set error, because FVAULT2 technology is not available */ + return bd_crypto_is_tech_avail (BD_CRYPTO_TECH_FVAULT2, BD_CRYPTO_TECH_MODE_OPEN_CLOSE, error); +} +#else gboolean bd_crypto_fvault2_open (const gchar *device, const gchar *name, BDCryptoKeyslotContext *context, gboolean read_only, GError **error) { return bd_crypto_fvault2_open_flags (device, name, context, read_only ? BD_CRYPTO_OPEN_READONLY : 0, error); } #endif
♻️ Duplicate comments (1)
src/lib/plugin_apis/crypto.api (1)
947-970: Fixbd_crypto_luks_open_flagsdocumentation exampleThe example block still says “using %bd_crypto_luks_open” even though it demonstrates
bd_crypto_luks_open_flags, and it passes0as flags, which doesn’t showcase the new API.Consider updating the example title and using a real flag, e.g.:
- * Example of using %bd_crypto_luks_open with %BDCryptoKeyslotContext: + * Example of using %bd_crypto_luks_open_flags with %BDCryptoKeyslotContext: @@ - * context = bd_crypto_keyslot_context_new_passphrase ("passphrase", 10, NULL); - * bd_crypto_luks_open_flags ("/dev/vda1", "luks-device", context, 0, NULL); + * context = bd_crypto_keyslot_context_new_passphrase ("passphrase", 10, NULL); + * bd_crypto_luks_open_flags ("/dev/vda1", "luks-device", context, BD_CRYPTO_OPEN_READONLY, NULL);This keeps the example aligned with the function name and better illustrates how to use the flag bitfield.
🧹 Nitpick comments (1)
src/plugins/crypto.c (1)
1265-1401: LUKS_open_flagsimplementation looks correct; doc example header still names the old function.The new
bd_crypto_luks_open_flags()helper cleanly translatesBDCryptoOpenFlagsintocrypt_flagsand reuses the existing activation paths; the wrapperbd_crypto_luks_open()preserves the previousread_onlyAPI while delegating to the flags-based variant.The only nit is in the doc block for
bd_crypto_luks_open_flags: the example header still says “Example of using %bd_crypto_luks_open…” even though the example actually callsbd_crypto_luks_open_flags(). Updating that symbol name in the comment would avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/libblockdev-sections.txt(2 hunks)src/lib/plugin_apis/crypto.api(5 hunks)src/plugins/crypto.c(20 hunks)src/plugins/crypto.h(3 hunks)src/python/gi/overrides/BlockDev.py(1 hunks)tests/crypto_test.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/libblockdev-sections.txt
- src/python/gi/overrides/BlockDev.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/crypto_test.py (2)
src/python/gi/overrides/BlockDev.py (2)
CryptoKeyslotContext(284-300)crypto_tc_open_flags(341-344)tests/utils.py (1)
run_command(561-566)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: compilation (clang-17)
- GitHub Check: compilation (clang-18)
- GitHub Check: compilation (gcc-14)
- GitHub Check: compilation (clang-15)
- GitHub Check: compilation (clang-16)
- GitHub Check: compilation (clang-14)
- GitHub Check: compilation (gcc-10)
- GitHub Check: compilation (gcc-13)
- GitHub Check: compilation (gcc-11)
- GitHub Check: compilation (gcc-12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: blivet-tests
- GitHub Check: udisks-build
- GitHub Check: symbols
- GitHub Check: csmock
🔇 Additional comments (16)
src/lib/plugin_apis/crypto.api (4)
364-367: BDCryptoOpenFlags definition matches usage and headerEnum values for
BD_CRYPTO_OPEN_ALLOW_DISCARDSandBD_CRYPTO_OPEN_READONLYare well‑chosen and consistent with other flag enums in this API; no issues from an ABI/API standpoint.
1333-1352: TrueCrypt/VeraCrypt_open_flagsAPI and docs look consistentThe
bd_crypto_tc_open_flagsprototype, parameter order, and documentation (including@flagsand supported context types) are consistent with the rest of the crypto API and withbd_crypto_tc_open.
1396-1411: BitLocker_open_flagsAPI mirrors existing open behavior
bd_crypto_bitlk_open_flagscleanly extendsbd_crypto_bitlk_openby replacing the booleanread_onlywithBDCryptoOpenFlags flags, while keeping context handling and tech category docs consistent.
1439-1453: FileVault2_open_flagsAPI is consistent with other flag-based opens
bd_crypto_fvault2_open_flagsfollows the same pattern as the LUKS/TrueCrypt/BitLocker variants; the docs correctly describe@flagsand supported context types.tests/crypto_test.py (5)
120-127: _is_ro helper correctly inspects dm read-only stateUsing
/sys/block/<dm-name>/rovia the real/dev/mapper/<name>path is a straightforward and reliable way to assert RO status for dm devices; this is appropriate for these tests.
714-742: LUKS_open_flagstests now cover both discards and read-only
CryptoTestLuksOpenFlagsexercises bothALLOW_DISCARDSandREADONLY, checksdmsetup tableforallow_discards, and uses_is_roto validate read-only state for both LUKS1 and LUKS2. This gives solid coverage of the new flag behavior.
1522-1535: VeraCrypt_open_flagstest validates flags end-to-endThe added
crypto_tc_open_flagstest opens the VeraCrypt volume withALLOW_DISCARDS | READONLY, then checksdmsetupoutput and_is_ro("libblockdevTestTC"). This is a good end‑to‑end verification of the new flags for TrueCrypt/VeraCrypt mappings.
1637-1650: BitLocker_open_flagstest aligns with feature goalsFor BitLocker, the test reuses the existing open/close flow and then re-opens with
CryptoOpenFlags.ALLOW_DISCARDS | READONLY, validating bothallow_discardsin the dm table and the RO state via_is_ro. This correctly exercises the new flags on BITLK devices (gated byHAVE_BITLK).
1751-1764: FileVault2_open_flagstest completes coverage across device typesThe FileVault2 test mirrors the BitLocker pattern: open with flags, assert
allow_discardsand RO state, and cleanly close. Together with the LUKS, TrueCrypt, and BITLK tests, this gives uniform coverage ofBDCryptoOpenFlagsacross all supported techs.src/plugins/crypto.h (5)
175-178: BDCryptoOpenFlags enum is consistent with public API
BDCryptoOpenFlagshere matches the definition insrc/lib/plugin_apis/crypto.api(same bits and members), so consumers see a coherent flag type across both plugin and public headers.
296-299: LUKS_open_flagsdeclaration is a clean extension
bd_crypto_luks_open_flagsreplaces the booleanread_onlywithBDCryptoOpenFlags flagswhile keeping the rest of the signature stable, which is a backward‑compatible and predictable evolution of the API.
326-329: TrueCrypt_open_flagsprototype matches the documented API
bd_crypto_tc_open_flagsmirrorsbd_crypto_tc_open’s parameters and simply swaps theread_onlyboolean forBDCryptoOpenFlags flagsat the end, matching the ordering and semantics described incrypto.api.
331-333: BitLocker_open_flagsdeclaration is consistent with existing open
bd_crypto_bitlk_open_flagsfollows the same pattern as the LUKS and TC variants, preserving the existing calling convention and addingBDCryptoOpenFlagsas a drop‑in replacement forread_only.
335-337: FileVault2_open_flagsdeclaration completes the set
bd_crypto_fvault2_open_flagsextends the FileVault2 open API in the same way as the other technologies, keeping consistency across the crypto plugin surface.src/plugins/crypto.c (2)
3194-3326: TrueCrypt/VeraCrypt_open_flagshelper and wrapper are consistent with existing semantics.
bd_crypto_tc_open_flags()properly validates the optional passphrase context, counts keyfiles, mapsBDCryptoOpenFlagsto an internalcrypt_flagsbitmask, and then activates via the existing path.bd_crypto_tc_open()now just delegates to the new helper with aread_only→BD_CRYPTO_OPEN_READONLYmapping, so existing callers retain behavior while gaining the option to use the richer flags API.
3589-3705: BITLK_open_flagsplumbing mirrors the LUKS path and keeps the old API intact.
bd_crypto_bitlk_open_flags()follows the same pattern as the LUKS open helper: validate the DM name, load the device, derivecrypt_flagsfromBDCryptoOpenFlags, and reuse the existing passphrase/keyfile activation logic. Thebd_crypto_bitlk_open()wrapper’sread_only→BD_CRYPTO_OPEN_READONLYmapping preserves previous semantics while allowing callers to opt into the new flags-based entry point.
This currently allows setting the device as read only and enabling discard for all types of devices supported by cryptsetup. Fixes: storaged-project#1138
vojtechtrefny
force-pushed
the
master_luks-open-flags
branch
from
c3415f1 to
8338945
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/crypto_test.py (1)
714-741: Consider testing flags individually for better coverage.The test correctly verifies both
ALLOW_DISCARDSandREADONLYflags together. For more comprehensive coverage, consider adding separate test cases that verify each flag individually:
- Test with only
ALLOW_DISCARDS- Test with only
READONLY- Test with both combined (current implementation)
This would ensure each flag works correctly in isolation and in combination.
src/lib/plugin_apis/crypto.api (1)
947-970: Improve documentation example to demonstrate flag usage.The documentation example currently passes
0for the flags parameter, which is valid but doesn't demonstrate the purpose of the new function. Consider showing an actual flag being used to make the example more illustrative:context = bd_crypto_keyslot_context_new_passphrase ("passphrase", 10, NULL); bd_crypto_luks_open_flags ("/dev/vda1", "luks-device", context, BD_CRYPTO_OPEN_READONLY | BD_CRYPTO_OPEN_ALLOW_DISCARDS, NULL);This would better demonstrate how to use the flags parameter in practice.
src/plugins/crypto.c (3)
1265-1401: LUKS_open_flagsimplementation looks correct; consider a tiny doc/example tweak and flag helper.The flag handling and activation calls for
bd_crypto_luks_open_flagsmirror existing patterns and look correct. Two small, optionally-addressable nits:
- The example still uses
flags = 0; consider demonstratingBD_CRYPTO_OPEN_ALLOW_DISCARDSorBD_CRYPTO_OPEN_READONLYto highlight the new capability.- The
BDCryptoOpenFlags→crypt_flagsmapping is duplicated in other_open_flagsfunctions; a small static helper (e.g._bd_crypto_open_flags_to_crypt_flags()) would reduce duplication and keep behavior in one place.
3194-3326: TrueCrypt/VeraCrypt_open_flagsvariant is consistent; only minor reuse opportunity.
bd_crypto_tc_open_flagscorrectly:
- Validates the dm name and key material.
- Preserves existing behavior via
crypt_params_tcryptandcrypt_load(CRYPT_TCRYPT, ¶ms).- Applies
BDCryptoOpenFlagsonly to the activation step viacrypt_activate_by_volume_key.Same as for LUKS, the local
crypt_flagsconstruction is identical in several_open_flagsfunctions; factoring it into a shared helper would simplify future maintenance without changing semantics.
3589-3705: BITLK_open_flagsand wrapper maintain existing semantics; flags mapping is fine but duplicated.
bd_crypto_bitlk_open_flagsandbd_crypto_bitlk_open:
- Keep the previous “read-only vs read-write” behavior through the wrapper.
- Correctly propagate
BD_CRYPTO_OPEN_ALLOW_DISCARDSandBD_CRYPTO_OPEN_READONLYtocrypt_activate_by_passphrase.Again, the only repetition is the
BDCryptoOpenFlags→crypt_flagsconversion, which matches other call sites; extracting a small helper for that mapping would DRY things up.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/libblockdev-sections.txt(2 hunks)src/lib/plugin_apis/crypto.api(5 hunks)src/plugins/crypto.c(20 hunks)src/plugins/crypto.h(3 hunks)src/python/gi/overrides/BlockDev.py(1 hunks)tests/crypto_test.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/libblockdev-sections.txt
- src/python/gi/overrides/BlockDev.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/crypto.h (1)
src/plugins/crypto.c (5)
bd_crypto_luks_open_flags(1287-1374)bd_crypto_tc_open_flags(3212-3303)bd_crypto_fvault2_open(3843-3847)bd_crypto_fvault2_open(3849-3851)bd_crypto_fvault2_open_flags(3740-3826)
tests/crypto_test.py (2)
src/python/gi/overrides/BlockDev.py (2)
CryptoKeyslotContext(284-300)crypto_tc_open_flags(341-344)tests/utils.py (1)
run_command(561-566)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: compilation (clang-16)
- GitHub Check: compilation (clang-18)
- GitHub Check: compilation (clang-14)
- GitHub Check: compilation (gcc-12)
- GitHub Check: compilation (clang-17)
- GitHub Check: compilation (gcc-11)
- GitHub Check: compilation (gcc-10)
- GitHub Check: compilation (clang-15)
- GitHub Check: compilation (gcc-14)
- GitHub Check: compilation (gcc-13)
- GitHub Check: symbols
- GitHub Check: blivet-tests
- GitHub Check: csmock
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: udisks-build
🔇 Additional comments (6)
tests/crypto_test.py (2)
120-127: LGTM! Helper correctly checks device read-only state.The
_is_rohelper properly reads from sysfs to determine if a mapped device is read-only. The implementation correctly resolves the device path and constructs the appropriate sysfs path.
1522-1535: LGTM! Consistent test coverage across crypto formats.The open-with-flags tests for VeraCrypt, BitLocker, and FileVault2 follow the same pattern as the LUKS tests, properly verifying both the
ALLOW_DISCARDSflag (via dmsetup) and theREADONLYflag (via the_is_rohelper). The consistent structure makes the tests easy to understand and maintain.Also applies to: 1637-1650, 1751-1764
src/lib/plugin_apis/crypto.api (2)
364-367: LGTM! Enum properly defines activation flags.The
BDCryptoOpenFlagsenum is correctly defined with proper bitfield values, allowing flags to be combined using bitwise OR operations. The naming is clear and consistent with other enums in the codebase.
1333-1352: LGTM! Consistent API design across crypto formats.The
*_open_flagsfunction declarations for TrueCrypt, BitLocker, and FileVault2 are well-structured and consistent with each other. Each properly accepts theBDCryptoOpenFlagsparameter and includes complete documentation.Also applies to: 1396-1410, 1439-1453
src/plugins/crypto.h (1)
175-178: LGTM! Header properly declares new flags API.The
BDCryptoOpenFlagsenum and the four new*_open_flagsfunction declarations are correctly defined and consistent with the API definitions incrypto.api. The existing*_openfunctions remain unchanged, preserving backward compatibility while providing the new flag-based alternatives.Also applies to: 298-298, 328-328, 332-332, 336-336
src/plugins/crypto.c (1)
3721-3852: FVAULT2_open_flagslogic matches other backends; please double‑check header declaration.The FVAULT2 changes look good overall:
- The
#ifndef LIBCRYPTSETUP_26stubs for bothbd_crypto_fvault2_open_flagsandbd_crypto_fvault2_opencorrectly route tobd_crypto_is_tech_avail.- The real implementations follow the same keyfile/passphrase and error‑handling patterns as BITLK.
BDCryptoOpenFlagsare translated toCRYPT_ACTIVATE_*as expected.One thing to verify outside this file: ensure the declaration in
crypto.h(and any public API description files) uses exactly the same name and signature as this implementation, i.e.gboolean bd_crypto_fvault2_open_flags(..., BDCryptoOpenFlags flags, GError **error). A mismatch (for example a stray_FLAGSspelling or different parameter order) would break compilation or introspection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing activation flags to be used when opening various types of encrypted devices. The implementation is consistent across LUKS, TrueCrypt/VeraCrypt, BitLocker, and FileVault2, and includes corresponding API documentation, Python bindings, and tests. My review identifies a few opportunities to improve code maintainability and API consistency. Specifically, I've suggested refactoring duplicated code in the C implementation and tests into helper functions, and adding missing Python overrides for the new functions to ensure a consistent developer experience.
| if (flags & BD_CRYPTO_OPEN_ALLOW_DISCARDS) | ||
| crypt_flags |= CRYPT_ACTIVATE_ALLOW_DISCARDS; | ||
| if (flags & BD_CRYPTO_OPEN_READONLY) | ||
| crypt_flags |= CRYPT_ACTIVATE_READONLY; |
There was a problem hiding this comment.
This flag conversion logic is duplicated in bd_crypto_tc_open_flags, bd_crypto_bitlk_open_flags, and bd_crypto_fvault2_open_flags. To improve maintainability and reduce code duplication, consider extracting this logic into a static helper function.
For example, you could define a function like this near other static helpers (e.g., after _is_dm_name_valid):
static guint32
_bd_crypto_get_activate_flags (BDCryptoOpenFlags flags)
{
guint32 crypt_flags = 0;
if (flags & BD_CRYPTO_OPEN_ALLOW_DISCARDS)
crypt_flags |= CRYPT_ACTIVATE_ALLOW_DISCARDS;
if (flags & BD_CRYPTO_OPEN_READONLY)
crypt_flags |= CRYPT_ACTIVATE_READONLY;
return crypt_flags;
}Then, you can replace the declaration guint32 crypt_flags = 0; (line 1295) with guint32 crypt_flags = _bd_crypto_get_activate_flags(flags); and remove this block of if statements. This refactoring should be applied to all new ..._open_flags functions to centralize the flag handling.
There was a problem hiding this comment.
well, you should've suggested static inline func instead, or a macro! Too much context switching here.
| _crypto_tc_open_flags = BlockDev.crypto_tc_open_flags | ||
| @override(BlockDev.crypto_tc_open_flags) | ||
| def crypto_tc_open_flags(device, name, passphrase, flags=0, keyfiles=None, hidden=False, system=False, veracrypt=False, veracrypt_pim=0): | ||
| if isinstance(passphrase, str): | ||
| passphrase = passphrase.encode("utf-8") | ||
| return _crypto_tc_open_flags(device, name, passphrase, keyfiles, hidden, system, veracrypt, veracrypt_pim, flags) | ||
| __all__.append("crypto_tc_open_flags") |
There was a problem hiding this comment.
This pull request introduces new _flags versions for opening encrypted devices (luks, tc, bitlk, fvault2), but an override is only provided for crypto_tc_open_flags. For consistency and ease of use, you should also provide overrides for crypto_luks_open_flags, crypto_bitlk_open_flags, and crypto_fvault2_open_flags.
Without these overrides, users will have to manually create a CryptoKeyslotContext object for these functions, which is inconsistent with the existing _open functions that accept a passphrase string directly.
For example, for LUKS, you could add:
_crypto_luks_open_flags = BlockDev.crypto_luks_open_flags
@override(BlockDev.crypto_luks_open_flags)
def crypto_luks_open_flags(device, name, passphrase, flags=0):
if isinstance(passphrase, str):
passphrase = passphrase.encode("utf-8")
return _crypto_luks_open_flags(device, name, passphrase, flags)
__all__.append("crypto_luks_open_flags")Similar overrides should be added for bitlk and fvault2.
| # check that discard is enabled for the mapped device | ||
| _ret, out, _err = run_command("dmsetup table %s" % self._dm_name) | ||
| self.assertIn("allow_discards", out) | ||
|
|
||
| # check that the device is read-only | ||
| self.assertTrue(self._is_ro("libblockdevTestLUKS")) |
There was a problem hiding this comment.
This block of code for checking allow_discards and read-only status is duplicated in test_veracrypt_open_close, test_bitlk_open_close, and test_fvault2_open_close. To improve test code maintainability, consider extracting this logic into a helper method within the CryptoTestCase class.
For example:
def _check_open_flags(self, dm_name):
# check that discard is enabled for the mapped device
_ret, out, _err = run_command("dmsetup table %s" % dm_name)
self.assertIn("allow_discards", out)
# check that the device is read-only
self.assertTrue(self._is_ro(dm_name))Then you can call self._check_open_flags("libblockdevTestLUKS") here and in the other test methods.
| if (flags & BD_CRYPTO_OPEN_ALLOW_DISCARDS) | ||
| crypt_flags |= CRYPT_ACTIVATE_ALLOW_DISCARDS; | ||
| if (flags & BD_CRYPTO_OPEN_READONLY) | ||
| crypt_flags |= CRYPT_ACTIVATE_READONLY; |
There was a problem hiding this comment.
well, you should've suggested static inline func instead, or a macro! Too much context switching here.
| BD_CRYPTO_LUKS_ACTIVATE_HIGH_PRIORITY = 1 << 6, | ||
| } BDCryptoLUKSPersistentFlags; | ||
|
|
||
| typedef enum { |
2 participants
This currently allows setting the device as read only and enabling discard for all types of devices supported by cryptsetup.
Fixes: #1138
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.