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

Add support for mounting /boot #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for mounting /boot #119

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion grml-debootstrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ Bootstrap options:
-r, --release <name> Release of new Debian system (default: buster).
-t, --target <target> Target partition (/dev/...) or directory where the
system should be installed to.
--mountboot <device> Target partition (/dev/...) or directory where the
/boot should be mounted to.
-p, --mntpoint <mnt> Mountpoint used for mounting the target system,
has no effect if -t is given and represents a directory.
--debopt <params> Extra parameters passed to the debootstrap command.
Expand Down Expand Up @@ -372,6 +374,9 @@ while :; do
--target|-t) # Target partition (/dev/...) or directory
shift; _opt_target="$1"
;;
--mountboot) # Target partition (/dev/...) or directory
shift; _opt_mountboot="$1"
Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to add it to the getopt handling.

;;
--vm) # Virtual machine image (no file)
_opt_vm="T"
;;
Expand Down Expand Up @@ -563,6 +568,7 @@ done
[ "$_opt_bootappend" ] && BOOT_APPEND=$_opt_bootappend
[ "$_opt_grub" ] && GRUB=$_opt_grub
[ "$_opt_efi" ] && EFI=$_opt_efi
[ "$_opt_mountboot" ] && MOUNT_BOOT=$_opt_mountboot
[ "$_opt_arch" ] && ARCH=$_opt_arch
[ "$_opt_insecure" ] && echo "Warning: --insecure is deprecated, continuing anyway."
[ "$_opt_force" ] && FORCE=$_opt_force
Expand Down Expand Up @@ -1385,6 +1391,26 @@ mount_target() {
}
# }}}

# mount the new partition or if it's a directory do nothing at all {{{
mount_boot_target() {
if [ -n "$MOUNT_BOOT" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the wrong logic (as in: shouldn't this be -z ... here)?
Also we should consider verifying it's actually a valid block/LVM/... device, or at least that it can be accessed.
Since we don't format it on our own we should also check for a valid/supported filesystem?

While at it, please use "${MOUNT_BOOT}" instead of "$MOUNT_BOOT" etc. and two-space indention everywhere, that's the coding style we try to follow nowadays (yes, I'm aware that this is still inconsistent in our scripts :-/).

einfo "/boot is not on a separate partition, nothing to mount."
Copy link
Member

Choose a reason for hiding this comment

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

You can just "return 0" afterwards and don't need the else ... afterwards, since nothing from this function needs to be executed if separate /boot isn't requested.

else
if grep -q "$MOUNT_BOOT" /proc/mounts ; then
ewarn "$MOUNT_BOOT already mounted, continuing anyway." ; eend 0
else
if ! [ -d "${MNTPOINT}/boot" ] ; then
[ -n "$VIRTUAL" ] || mkdir -p "${MNTPOINT}/boot"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Those three lines feel unnecessary, given that /boot should exist anyway and that you're creating it in line 1406 anyway :)

einfo "Mounting $MOUNT_BOOT to $MNTPOINT/boot"
mkdir -p "$MNTPOINT/boot"
mount -o rw,suid,dev "$MOUNT_BOOT" "$MNTPOINT/boot"
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we need suid + dev permissions on /boot?
Also we should fail better with an actual error message.

eend $?
fi
fi
}
# }}}

# prepare VM image for usage with debootstrap {{{
prepare_vm() {
if [ -z "$VIRTUAL" ] ; then
Expand Down Expand Up @@ -1642,6 +1668,7 @@ preparechroot() {
[ -n "$TARGET" ] && echo "TARGET='$(sed "s,','\\\\'',g" <<<"${TARGET}")'" >> "$CHROOT_VARIABLES"
[ -n "$UPGRADE_SYSTEM" ] && echo "UPGRADE_SYSTEM='$(sed "s,','\\\\'',g" <<<"${UPGRADE_SYSTEM}")'" >> "$CHROOT_VARIABLES"
[ -n "$TARGET_UUID" ] && echo "TARGET_UUID='$(sed "s,','\\\\'',g" <<<"${TARGET_UUID}")'" >> "$CHROOT_VARIABLES"
[ -n "$BOOT_UUID" ] && echo "BOOT_UUID='$(sed "s,','\\\\'',g" <<<"${BOOT_UUID}")'" >> "$CHROOT_VARIABLES"
Copy link
Member

Choose a reason for hiding this comment

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

This is unused and not yet handled inside chroot-script?

[ -n "$TIMEZONE" ] && echo "TIMEZONE='$(sed "s,','\\\\'',g" <<<"${TIMEZONE}")'" >> "$CHROOT_VARIABLES"
[ -n "$TUNE2FS" ] && echo "TUNE2FS='$(sed "s,','\\\\'',g" <<<"${TUNE2FS}")'" >> "$CHROOT_VARIABLES"
[ -n "$VMSIZE" ] && echo "VMSIZE='$(sed "s,','\\\\'',g" <<<"${VMSIZE}")'" >> "$CHROOT_VARIABLES"
Expand Down Expand Up @@ -1824,6 +1851,7 @@ try_umount() {
chrootscript() {
if ! [ -r "$MNTPOINT/bin/chroot-script" ] ; then
mount_target
mount_boot_target
fi

if ! [ -x "$MNTPOINT/bin/chroot-script" ] ; then
Expand Down Expand Up @@ -1923,7 +1951,7 @@ remove_configs() {

# now execute all the functions {{{
for i in format_efi_partition prepare_vm mkfs tunefs \
mount_target mountpoint_to_blockdevice debootstrap_system \
mount_target mount_boot_target mountpoint_to_blockdevice debootstrap_system \
preparechroot execute_pre_scripts chrootscript execute_post_scripts \
remove_configs umount_chroot grub_install umount_target fscktool ; do
if stage "${i}" ; then
Expand Down