Conversation
|
This still needs to be tested, and should be considered as an RFC. |
Often, a separate boot partition is still desired, especially, when encrypting the root partition.
paulmenzel
force-pushed
the
allow-boot-on-separate-partition
branch
from
8e3d166 to
7e502d8
Compare
|
Creating the FS on the block device, and handling, if it’s a directory, is still missing. |
mika
left a comment
mika
left a comment
There was a problem hiding this comment.
Thanks for working on this, some preliminary comments while briefly going through the code.
| else | ||
| if ! [ -d "${MNTPOINT}/boot" ] ; then | ||
| [ -n "$VIRTUAL" ] || mkdir -p "${MNTPOINT}/boot" | ||
| fi |
There was a problem hiding this comment.
Those three lines feel unnecessary, given that /boot should exist anyway and that you're creating it in line 1406 anyway :)
| fi | ||
| einfo "Mounting $MOUNT_BOOT to $MNTPOINT/boot" | ||
| mkdir -p "$MNTPOINT/boot" | ||
| mount -o rw,suid,dev "$MOUNT_BOOT" "$MNTPOINT/boot" |
There was a problem hiding this comment.
Are you sure we need suid + dev permissions on /boot?
Also we should fail better with an actual error message.
|
|
||
| # mount the new partition or if it's a directory do nothing at all {{{ | ||
| mount_boot_target() { | ||
| if [ -n "$MOUNT_BOOT" ] ; then |
There was a problem hiding this comment.
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 :-/).
| [ -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" |
There was a problem hiding this comment.
This is unused and not yet handled inside chroot-script?
| shift; _opt_target="$1" | ||
| ;; | ||
| --mountboot) # Target partition (/dev/...) or directory | ||
| shift; _opt_mountboot="$1" |
There was a problem hiding this comment.
I think you forgot to add it to the getopt handling.
| # mount the new partition or if it's a directory do nothing at all {{{ | ||
| mount_boot_target() { | ||
| if [ -n "$MOUNT_BOOT" ] ; then | ||
| einfo "/boot is not on a separate partition, nothing to mount." |
There was a problem hiding this comment.
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.
|
@paulmenzel ping, are you still interested in getting this submitted? :) |
|
I stumbled over this today, so it would be really nice if we can get this into grml-debootstrap. :) |
3 participants
No description provided.