fix(extensions/lvm): Fix support for non-ext filesystems#9889
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesFilesystem-aware LVM partition labeling
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/lvm.sh`:
- Around line 75-76: The script checks ROOTFS_TYPE against "nilfs" but upstream
uses "nilfs2", so update both occurrences to use "nilfs2" so ROOTFS_TYPE=nilfs2
is recognized; specifically change the conditional branch and the nilfs-tune
invocation that currently reference "nilfs" to "nilfs2" (the lines handling
ROOTFS_TYPE and the nilfs-tune call that targets
"/dev/mapper/${LVM_VG_NAME}-root") so the correct filesystem type is detected
and tuned.
- Around line 72-78: The label commands in extensions/lvm.sh currently hardcode
"armbi_root" for different filesystems; update each invocation (the e2label
call, the btrfs filesystem label call that uses mount+awk, the nilfs-tune -L
call, and the xfs_io -c "label -s" call) to use the configured ROOT_FS_LABEL
variable instead (e.g. "$ROOT_FS_LABEL"), ensuring proper quoting and that you
still reference the device or mountpoint via LVM_VG_NAME or the mount pipeline
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7eaee1e2-dcdd-4c39-a2ee-000088069330
📒 Files selected for processing (1)
extensions/lvm.sh
| function label_partition() { | ||
| if [ "$ROOTFS_TYPE" == "ext4" ]; then | ||
| e2label "/dev/mapper/${LVM_VG_NAME}-root" "${ROOT_FS_LABEL}" | ||
| elif [ "$ROOTFS_TYPE" == "btrfs" ]; then | ||
| btrfs filesystem label "$(mount | grep "/dev/mapper/${LVM_VG_NAME}-root" | awk '{print $3}')" "${ROOT_FS_LABEL}" | ||
| elif [ "$ROOTFS_TYPE" == "nilfs2" ]; then | ||
| nilfs-tune -L "${ROOT_FS_LABEL}" "/dev/mapper/${LVM_VG_NAME}-root" | ||
| elif [ "$ROOTFS_TYPE" == "xfs" ]; then | ||
| xfs_io -c "label -s \"${ROOT_FS_LABEL}\"" "$(mount | grep "/dev/mapper/${LVM_VG_NAME}-root" | awk '{print $3}')" | ||
| fi |
There was a problem hiding this comment.
perhaps the design of the "case" for selection here would be clearer and more compact
There was a problem hiding this comment.
like that
function format_partitions__format_lvm() {
local rootdev="/dev/mapper/${LVM_VG_NAME}-root"
local mountpoint
case "${ROOTFS_TYPE}" in
ext4|ext2) e2label "${rootdev}" "${ROOT_FS_LABEL}" ;;
btrfs)
mountpoint=$(findmnt -no TARGET "${rootdev}")
btrfs filesystem label "${mountpoint}" "${ROOT_FS_LABEL}
nilfs2) nilfs-tune -L "${ROOT_FS_LABEL}" "${rootdev}" ;;
xfs)
mountpoint=$(findmnt -no TARGET "${rootdev}")
xfs_io -c "label -s ${ROOT_FS_LABEL}" "${mountpoint}" ;;
*) display_alert "LVM label skipped for ${ROOTFS_TYPE:-unset}" "
"info"
return 0 ;;
esac \
&& { blkid | grep -F "${LVM_VG_NAME}" >> "${DEST}/${LOG_SUBPATH}
display_alert "LVM labeled ${ROOTFS_TYPE} partition" "${EXT
"info"; } \
|| display_alert "LVM failed to label ${ROOTFS_TYPE} partition.
"${EXTENSION}" "info"
}
| elif [ "$ROOTFS_TYPE" == "nilfs2" ]; then | ||
| nilfs-tune -L "${ROOT_FS_LABEL}" "/dev/mapper/${LVM_VG_NAME}-root" | ||
| elif [ "$ROOTFS_TYPE" == "xfs" ]; then | ||
| xfs_io -c "label -s \"${ROOT_FS_LABEL}\"" "$(mount | grep "/dev/mapper/${LVM_VG_NAME}-root" | awk '{print $3}')" |
There was a problem hiding this comment.
xfs_io -c "label -s ${ROOT_FS_LABEL}" "${mountpoint}"
simpler and safer
| } | ||
|
|
||
| function label_partition() { | ||
| if [ "$ROOTFS_TYPE" == "ext4" ]; then |
|
findmnt — structured, reliable, single-process method; mount | grep | awk — heuristic from 1990s. |
2 participants
Description
Compiling an image with ROOTFS_TYPE=btrfs and ENABLE_EXTENSIONS=lvm fails, because the lvm extensions tries to add a label to the filesystem using a ext4 specific command.
This PR differentiates between the different filesystems and uses the appropriate command for each to add the label.
nfs and f2fs are not labled, as they do not support labels, or labels cannot be added while the filesystem is mounted.
If the label cannot be successfully added, a message is printed, but ignored.
How Has This Been Tested?
Checklist:
Summary by CodeRabbit