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

gpt-auto-generator: correctly handle the return value of function slash_boot_in_fstab() #28652

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 2, 2023

Fixes: #28585

Actually, the return value of slash_boot_in_fstab() maybe true:

return true;

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Aug 2, 2023
Comment on lines 589 to 591
r = slash_boot_in_fstab();
if (r < 0)
return r;
Copy link
Member

Choose a reason for hiding this comment

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

This checks if /boot is in fstab, and return true if it exists. Ignoring non-negative value is meaningless, and if this really fixes the issue, then please drop the call entirely.

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks gpt-auto and removed please-review PR is ready for (re-)review by a maintainer labels Aug 4, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 5, 2023
@ghost ghost requested a review from yuwata August 5, 2023 03:15
@@ -586,17 +586,12 @@ static int add_partition_esp(DissectedPartition *p, bool has_xbootldr) {
* Otherwise, if /efi/ is unused and empty (or missing), we'll take that.
* Otherwise, we do nothing. */
if (!has_xbootldr && slash_boot_exists()) {
r = slash_boot_in_fstab();
r = path_is_busy("/boot");
Copy link
Member

@YHNdnzj YHNdnzj Aug 5, 2023

Choose a reason for hiding this comment

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

This is wrong. If the ESP already has an entry in fstab, fstab_has_node above should handle it. If not, we should only use /boot/ when it's not specified in fstab, and fallback to /efi/ otherwise. You're just overmounting /boot/ if it's empty, regardless of already listed in fstab or not.

Copy link
Member

Choose a reason for hiding this comment

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

Note that although path_is_busy checks whether the path is already a mountpoint, no mount unit will have been started by the time the generator does its initial run, i.e. during systemd startup. So we need to check fstab explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

Here is the fstab on my system:

UUID=93fe18d5-1331-4d58-b6a5-fceb1d130300	/         	ext4      	rw,relatime	0 1
UUID=1DAD-F136      	/boot     	vfat      	rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=ascii,shortname=mixed,utf8,errors=remount-ro	0 2 

Does this mean that fstab_has_node is not as expected?

Copy link
Member

Choose a reason for hiding this comment

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

-> #28690

@YHNdnzj YHNdnzj added replaced-by-newer-pr and removed please-review PR is ready for (re-)review by a maintainer labels Aug 5, 2023
@ghost ghost closed this Aug 5, 2023
@ghost ghost deleted the fixed-28585 branch August 5, 2023 21:11
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

gpt-auto-generator duplicate mounts ESP at /boot and /efi
2 participants