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

packages/kata-image: specify correct fs-type in error message #1051

Closed
wants to merge 1 commit into from

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Dec 4, 2024

Previously, we used stat as a heuristic to detect the filesystem type, which is sub-optimal, as it will report ext2/ext3 on ext4 filesystems, leaving the reader of the error message in incertainty about the error. Switch to df, which is a better heuristic. [1]

[1]: https://unix.stackexchange.com/questions/256920/stat-f-show-a-ext4-file-system-type-as-ext2-ext3/274030#274030

Previously, we used `stat` as a heuristic to detect the filesystem type, which is sub-optimal, as it will report `ext2/ext3` on `ext4` filesystems, leaving the reader of the error message in incertainty about the error. Switch to `df`, which is a better heuristic. \[1\]

\[1\]: https://unix.stackexchange.com/questions/256920/stat-f-show-a-ext4-file-system-type-as-ext2-ext3/274030#274030
@msanft msanft requested a review from burgerdev December 4, 2024 08:44
@msanft msanft requested a review from katexochen as a code owner December 4, 2024 08:44
@@ -155,7 +155,7 @@ let
runHook preBuild

# Check if filesystem is ext.*
fstype=$(stat -f -c %T .)
fstype=$(df -T . | awk '{print $2}' | tail -n 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the issue with stat is that the superblock magic of ext4 is the same as for ext2 and ext3, so it lacks details to understand the diff and nobody bothered to upgrade the type string (or maybe it needs to stay like that for backwards compatibility). For our purposes, this does not really matter, since all ext* are affected. Could we just replace the $fstype in the error message with a literal ext2/ext3/ext4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought that this will be superfluous with #1019 anyways. Closing this one in favor of that.

@msanft msanft closed this Dec 4, 2024
@msanft msanft deleted the msanft/kata/error-message-fstype branch December 9, 2024 08:16
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.

2 participants