Hi Philipp,
On Fri, Jul 29, 2022 at 05:33:57PM +0200, Philipp Rudo wrote:
Hi Coiby,
this series doesn't seem to be merged yet. So let me add a few comments/questions ;-)
Thanks for reviewing the patch!
On Fri, 13 May 2022 14:08:08 +0800 Coiby Xu coxu@redhat.com wrote:
Fedora 33 and 34 Cloud Base Images have only partition with the
^ one (?)
Nice catch!
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?
/root/root is the home directory for root and /root/home is for everybody else. And /home seems to be unused.
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?
Sorry for the confusion! FIRST_PARTITION should be SECOND_PARTITION.
Signed-off-by: Coiby Xu coxu@redhat.com
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' ?
Yes, I should have simply used "ls -1 ${dev}". However your suggestion of using lsblk inspires me to find a more reliable method which also works for RHEL cloud base image,
if [[ $(lsblk -f ${dev}p2 -n -o LABEL 2> /dev/null) == boot ]] echo ${dev}p2 fi
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
Btw, the above command doesn't work for the cloud image case,
$ lsblk -p -r -n -o NAME,MOUNTPOINT /dev/nbd2 /dev/nbd2 /dev/nbd2p1 /dev/nbd2p2 /dev/nbd2p3 /dev/nbd2p4 /dev/nbd2p5
or lsblk -p -r -n -o NAME,PARTLABEL ${dev} | grep efi
So is the above command.
(what for non-efi systems, e.g. s390?).
Currently, Fedora doesn't have s390 cloud image so we simply skip this concern now.
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
}