Skip to content

Support cloning to a loopback device #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support cloning to a loopback device #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions rpi-clone
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,9 @@ then
else
assumed_fstab_edit=0
fi
elif [[ $dst_disk == *"loop"* ]]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great improvement of rpi-clone. But why didn't you just add the check for loop device in line 1066?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I wrote this, but looking at it now, the if at 1066 makes a couple more changes that do not seem appropriate for loop devices: It sets SD_slot_dst=1, but a loop device is not an SD card, and it sets edit_fstab_name, which triggers editing the fstab and cmdline.txt to (in the unlikely case that it still contains plain device names) update that to the loop device name, which is never really useful I think.

This does mean that there is one duplicate line (dst_part_base=${dst_disk}p), but I believe it's either that, or having a duplicate conditional for checking SD cards partiton names, so it seems the current commit is acceptable in terms of duplication.

Copy link

@framps framps Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

SD_slot_dst=1 is used as far as I understand to support hybrid boot environments which don't support USB boot and thus still require a SD card to provide the boot partition and later on switch according /etc/fstab to use an USB partition.

Did you test your PR with a hybrid environment when a loop device is used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test your PR with a hybrid environment when a loop device is used?

I did not, but the (my) usecase for a loop device is to generate an image that can later be written to an SD card again. And writing this, I considered it would maybe actually make sense to set SD_slot_dst=1 when writing to a loop device, since the image will likely end up on an SD card later.

However, since it is unclear where the image will end up (i.e. will it replace the currently running SD card in this system? Or maybe it will be written to an USB stick?), I wonder if the --leave-sd-usb-boot option (which is needed for SD_slot_dst to be used at all) does even make sense when used together with loop images.

Looking at the code (and the documentation), the --leave-sd-usb-boot option serves two usecases:

  1. When cloning SD-to-USB, replace the source cmdline.txt with a copy of the (updated-with-USB-rootfs-references) cloned cmdline.txt, ensuring that after the clone, the system will boot from the USB stick (i.e. the bootloader runs from the SD card, but then boots using the kernel and rootfs from the USB stick). This behavior is triggered by SD_slot_boot, which is set when the source device is an mmc/nvme device.
  2. When cloning USB-to-SD, do not modify the cloned cmdline.txt at all, ensuring after the clone the system will boot from the USB stick (because cmdline.txt will contain references to the USB stick PARTUUIDs). This behavior is triggered by SD_slot_dst, which is set when the destination device is an mmc/nvme device.

In both cases, a cmdline.boot file is also produced with the right values to boot from the SD card, to be manually restored to disable USB boot again.

Now, when cloning to an image, I do not think either of the above usecases would really apply. Also, neither my current PR nor your suggestion seem to make sense when combined with --leave-sd-usb-boot:

  • With my current PR, SD_slot_dst is not set, so when doing an SD-to-loop clone case 1 above would end up with an unbootable system, unless the image written is subsequently written to an USB stick or similar and inserted back into the device.
  • With your suggestion SD_slot_dst would be set, so when doing an SD-to-loop clone, SD_slot_boot is also set, so code for both cases above are triggered and I think the end result is an unbootable system and an unbootable image (by itself - it needs the unbootable system to boot).

So I guess that cloning to an image just does not make sense with --leave-sd-usb-boot, so maybe we could just raise an error on that combination and then not deal with it?

A part of the issue is that the --leave-sd-usb-boot really does two things, and which depends on whether the SD card is the source or destination. A different approach could be to split that option into two (something like --make-src-boot-dst and --make-dst-boot-src) and let the user decide which of these is appropriate. This might help making the code a bit easier to understand, but also allows someone that wants to clone to an image and that does need either of these usecases, to just be explicit about it so the code can comply with whatever they need (even if I cannot quickly come up with a meaningful reason why).

Note that in addition to setting SD_slot_dst, the code you referenced also sets assumed_fstab_edit which is actually unused, and edit_fstab_name, which causes direct disk references (e.g. /dev/sda1) in fstab to be updated to the SD card device name, even when the --edit-fstab option is not given. For a loop device, this really makes no sense - the loop device name is typically temporary and not available at boot time, so if you want to clone to a image file, you need to make sure you use PARTUUIDs (OTOH setting edit_fstab_name in this case is probably harmless - if using PARTUUIDs, the fstab editing will just be a no-op because the src device name will not be found).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm now convinced it doesn't make sense to handle hybrid boot with loop devices. I even think this support should be deprecated.

I tested your PR and detected there is a message Changing destination Disk ID ...Re-reading the partition table failed.: Invalid argument written which should be suppressed. This line should be modified from > to &>.

Other than that it works perfectly 👍

Copy link

@lexx9999 lexx9999 Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually trying the same thing.
My solution was to just add the 'p' line after the confirm when the device ends with a number.

confirm "Continue anyway?" "abort"
dst_part_base=${dst_disk}p

Despite I didn't found a proof I assume that partition numbers are always prefixed with a p if the device name ends with a number. Otherwise loop123 could be loop device 123 or loop device 12 partition 3 or loop device 1 partition 23.

I think the combination of both is probably the most correct solution. The 'p' line can be moved one level up. And the if is loop could also be negated, like:

if [[ ${chk_disk: -1} =~ ^[0-9]$ ]]
then
	dst_part_base=${dst_disk}p
	if [[ $dst_disk == *"mmcblk"* || $dst_disk == *"nvme"* ]]
	then
		SD_slot_dst=1
		if    [ "$edit_fstab_name" == "" ] \
		   && ! grep -q "^PARTUUID=" /etc/fstab
		then
			edit_fstab_name=$dst_part_base
			assumed_fstab_edit=1
		else
			assumed_fstab_edit=0
		fi
	elif [[ $dst_disk != *"loop"* ]]
 	then
		qecho $"
  Target disk $dst_disk ends with a digit so may be a partition.
  $PGM requires disk names like 'sda' and not partition names like 'sda1'."

		confirm "Continue anyway?" "abort"
	fi
fi

then
dst_part_base=${dst_disk}p
else
qecho $"
Target disk $dst_disk ends with a digit so may be a partition.
Expand Down