Hi Coiby,
On Tue, 2 Aug 2022 15:54:11 +0800 Coiby Xu coxu@redhat.com wrote:
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!
my pleasure.
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.
Thanks for the explanation! It's weird that there is this unused /home... but well it's not our problem ;-)
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 $_mntdone
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 ]; thenecho "$parts" | head -2 | tail -1fi- 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
Originally I refrained from using LABEL as the way I understand it it is more a scratchpad for users so they can remember which partition contains what. So especially on systems with a custom partition scheme there is no guarantee that the LABEL on the boot partition contains 'boot'. That's why I thought MOUNTPOINT or PARTLABEL are better choices (although when thinking about it PARTLABEL probably has the same problem).
Anyway, on second thought this script is only run in a very specific environment and isn't shipped to customers. So we don't have to be generic but can take some shortcuts. So using LABEL should be fine.
But I'm not sure if the suggestion you made above makes sense. Originally the function was used to return the boot partition of a device (which happens to be the second partition). Now you are verifying that the second partition is the boot partition.
Anyway when you are using the LABEL you can also use findfs to get the partition. I don't know what findfs does when there are multiple partitions with the same LABEL so I think it is better to pipe the output to grep. I.e. this should work
findfs LABEL=boot 2> /dev/null | grep $dev
Thanks Philipp
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
}