[Merge] ~gjolly/livecd-rootfs:ubuntu-cpc/boot_partition into livecd-rootfs:ubuntu/master

Steve Langasek mp+448592 at code.launchpad.net
Wed Sep 6 18:06:39 UTC 2023


Review: Needs Fixing

Minor point, but I don't think you should regress the comprehensibility of the code around setting image size limits

Diff comments:

> diff --git a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> index 020dd33..c687b0b 100755
> --- a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> +++ b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> @@ -12,17 +12,14 @@ esac
>  IMAGE_STR="# CLOUD_IMG: This file was created/modified by the Cloud Image build process"
>  FS_LABEL="cloudimg-rootfs"
>  
> -if [ "$ARCH" = "amd64" ]; then
> -    IMAGE_SIZE=3758096384 # bump to 3.5G (3584*1024**2); Since Kinetic amd64 need more then the default 2.2G
> -fi
> -
> -if [ "$ARCH" = "armhf" ]; then
> -    IMAGE_SIZE=3758096384 # bump to 3.5G (3584*1024**2); Since Jammy armhf need more then the default 2.2G
> -fi
> -
> -if [ "$ARCH" = "riscv64" ]; then
> -    IMAGE_SIZE=4831838208 # bump to 4.5G (4608*1024**2); initrd creation fails with "No space left" with 3.5G
> -fi
> +case "$ARCH" in
> +  amd64|arm64|armhf)
> +    IMAGE_SIZE=3758096384

some sort of annotation here about the meaning of this number would be helpful, for those of us that don't map such numbers from decimal to binary easily in our heads

or even just doing the math inline with $(( )) instead? $((3*1024*1024*1024+512*1024*1024))

> +    ;;
> +  riscv64)
> +    IMAGE_SIZE=4831838208
> +    ;;
> +esac
>  
>  . config/binary
>  
> @@ -60,15 +63,26 @@ create_partitions() {
>          --print
>  }
>  
> -create_and_mount_uefi_partition() {
> +create_and_mount_boot_partitions() {
>      uefi_dev="${loop_device}p15"
> +    boot_dev="${loop_device}p16"
>      mountpoint="$1"
> +
>      mkfs.vfat -F 32 -n UEFI "${uefi_dev}"
> +    mkfs.ext4 -L BOOT "${boot_dev}"
> +
> +    # copying what was on the rootfs to the new boot partition
> +    mount "${boot_dev}" "${mountpoint}"/mnt
> +    mv "${mountpoint}"/boot/* "${mountpoint}"/mnt
> +    umount "${boot_dev}"
> +
> +    mount "${boot_dev}" "${mountpoint}"/boot
>  
>      mkdir -p "${mountpoint}"/boot/efi
>      mount "${uefi_dev}" "$mountpoint"/boot/efi
>  
>      cat << EOF >> "mountpoint/etc/fstab"
> +LABEL=BOOT	/boot	ext4	defaults	0 2

I'm vaguely surprised to see mount by LABEL here, both for the ESP as well as the new /boot partition.  In non-cloud installs, we mount by UUID.  That gives more reliably-correct results for the case that the disk for one instance gets attached to another instance for debugging/recovery?

>  LABEL=UEFI	/boot/efi	vfat	umask=0077	0 1
>  EOF
>  }


-- 
https://code.launchpad.net/~gjolly/livecd-rootfs/+git/livecd-rootfs/+merge/448592
Your team Ubuntu Core Development Team is subscribed to branch livecd-rootfs:ubuntu/master.




More information about the Ubuntu-reviews mailing list