Re: [PATCH v3 1/3] tests: correctly mount the root and also the boot partitions for Fedora 35, 36 and rawhide Cloud Base Image

[Philipp Rudo]

Hi Coiby,

this series doesn't seem to be merged yet. So let me add a few
comments/questions ;-)

On Fri, 13 May 2022 14:08:08 +0800
Coiby Xu  wrote:
> Fedora 33 and 34 Cloud Base Images have only partition with the

^
    				     one (?)
> following directory structure,
> .
> ├── bin -> usr/bin
> ├── boot
> ├── dev
> ├── etc
> ├── home
> ├── root
>
> By comparison, Fedora 35, 36 and 37 Cloud Base Images have multiple
> partitions. The root partition which is the last partition has the
> following directory,
> .
> ├── home
> └── root
>     ├── bin -> usr/bin
>     ├── boot
>     ├── dev
>     ├── etc
>     ├── home
>     ├── root

Just for my understanding, in this schema /root/home is the home
directory for root and /home for everybody else. Is that correct?
> and the 2nd partition is the boot partition.
>
> This patch address the above changes by mounting {LAST_PARTITION}/root as
> to TEMP_ROOT and mount FIRST_PARTITION to TEMP_ROOT/boot. So the test
> image can be built successfully.

That's a little bit confusing to me. When the boot partition is the 2nd
partition why is the variable called FIRST_PARTITION?
> Signed-off-by: Coiby Xu
>
> tests/scripts/image-init-lib.sh | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/tests/scripts/image-init-lib.sh b/tests/scripts/image-init-lib.sh
> index 0a1524b..5995004 100644
> --- a/tests/scripts/image-init-lib.sh
> +++ b/tests/scripts/image-init-lib.sh
> @@ -23,7 +23,7 @@ is_mounted()
>  clean_up()
>  {
>    for _mnt in ${MNTS[@]}; do
> is_mounted $_mnt && $SUDO umount -f $_mnt
> is_mounted $_mnt && $SUDO umount -f -R $_mnt
>
> done
>
> for _dev in ${DEVS[@]}; do
>
> @@ -81,6 +81,21 @@ get_mountable_dev() {
>    fi
>  }
>
> +# get the separate boot partition
> +# return the 2nd partition as boot partition
> +get_mount_boot() {
> local dev=$1 parts
> $SUDO partprobe $dev && sync
> parts="$(ls -1 ${dev}p*)"
> if [ -n "$parts" ]; then
> if [ $(echo "$parts" | wc -l) -gt 1 ]; then
> 	echo "$parts" | head -2 | tail -1
> fi
> fi

Hmmm... difficult...

Can we really rely on ls providing the right order here? If so why not
simply do 'ls -1 ${dev}p2' ?

Would it make sense to switch to something more powerful like lsblk?
For example we could use

lsblk -p -r -n -o NAME,MOUNTPOINT ${dev} | grep $root/boot

or 
    lsblk -p -r -n -o NAME,PARTLABEL ${dev} | grep efi

(what for non-efi systems, e.g. s390?).

Finally, you can drop the two if-blocks by simply piping to sed. For
example your original version could be rewritten to

ls -1 ${dev}p* | sed -n 2p

which prints the second line or "" if it doesn't exist.

Thanks
Philipp
> +}
>
> prepare_loop() {
>    [ -n "$(lsmod | grep "^loop")" ] && return
>
> @@ -133,7 +148,7 @@ image_lock()
>  # Mount a device, will umount it automatially when shell exits
>  mount_image() {
>    local image=$1 fmt
> local dev mnt mnt_dev
>
> local dev mnt mnt_dev boot root
>
> # Lock the image just in case user run this script in parrel
> image_lock $image
>
> @@ -166,12 +181,20 @@ mount_image() {
>
> $SUDO mount $mnt_dev $mnt
>    [ $? -ne 0 ] && perror_exit "failed to mount device '$mnt_dev'"
> boot=$(get_mount_boot "$dev")
> if [[ -n "$boot" ]]; then
> root=$(get_image_mount_root $image)
> $SUDO mount $boot $root/boot
> [ $? -ne 0 ] && perror_exit "failed to mount the bootable partition for device '$mnt_dev'"
> fi
>
> }
>
> get_image_mount_root() {
>    local image=$1
>    local root=${MNTS[$image]}
>
> # Starting from Fedora 36, the root node is /root/root of the last partition
>
> [ -d "$root/root/root" ] && root=$root/root
> echo $root
>
> if [ -z "$root" ]; then
>
> @@ -213,7 +236,7 @@ run_in_image() {
>
> inst_in_image() {
>    local image=$1 src=$2 dst=$3
> local root=${MNTS[$image]}
>
> local root=$(get_image_mount_root $1)
>
> $SUDO cp $src $root/$dst
>
> }